Group-income-simple: Using modals

[sandrina-p]

Problem

Links vs Buttons

When a user clicks a link, and they are moved to another page (or are moved to another place within the same page), and the URL is updated, it's advised to use a proper link <a> instead of a <button> because it's what makes more sense semantically speaking.

Besides that, when a <button> is used instead of a proper link <a>, the user cannot do a task in a new tab if they want to (e.g: create a new group, change income details, etc...).

So far, we have been opening modals using a <button> and sbp. The modal is opened and the URL is also updated manually using $router.

button(@click='openModal("GroupCreationModal")') Add a group

// ...

openModal (name) {
  sbp('okTurtles.events/emit', OPEN_MODAL, name)
}

A solution is to use a proper link instead of a button when opening new modals. And we can use router-link without sbp.

router-link.link(:to='{ query: { modal: "GroupCreationModal" }}') Add a group

This approach is used by some other big websites: (e.g Twitter when creating a new tweet).

Props vs Queries

Some modals need specific data to be displayed correctly. Currently, we pass that data through props.

openModal () {
  sbp('okTurtles.events/emit', OPEN_MODAL, 'RemoveMember', { username: 'rick' })
}

And the URL gets updated, but without the data passed.

http://localhost:8000/app/dashboard?modal=RemoveMember

This approach has a flaw: when we refresh/open this link, the modal does not load because the prop is missing. After all, what's the point of changing the URL if the content (modal) does not get loaded as expected?

There are two solutions here:

1. Use queries instead of props.

I checked all modals and there are only 2 modals that need parameters (e.g. remove member and payment detail). That can be passed through a query:

http://localhost:8000/app/dashboard?modal=removeMember&username=rick
http://localhost:8000/app/dashboard?modal=paymentDetail&id=12345

There are some websites that follow this approach (e.g Twitter and Coinbase)

All routing libraries out there that I know do not allow passing props from route to route. Data can only be passed through queries. I guess it's because any route can be opened standalone without a previous context (e.g. open a link directly, or refresh the page). So, their approach is by using URL queries.

1. Do not update the URL when opening a modal.

This solves all the issues with URLs (and it's okay to use a <button> in that situation). However, we lose the possibility of opening in a new tab / sharing / refreshing a page with a modal. (related to bug #472).

Similar websites (Slack and Discord) use this approach. They do not update the URL when a modal is opened, never. Even in modals within modals.

---

There are still other issues related with modals (and related to #509)

1. If someones shares a link to create a proposal with a user that has multiple groups, the modal might open in the incorrect group and the user will create a proposal in an unexpected group.
   This is a problem, not only with modals, but with any page.

2. A modal can be shared with different contexts. For example, the link to UserSettings modal. When closing the modal, where do you expect to go? Dashboard, right? Well, in this particular link it goes to contributions... Maybe a modal (that changes the URL) should not be a query, but instead a route.

// a modal as a query
http://localhost:8000/app/contributions?modal=UserSettingsModal
// a modal as a route
http://localhost:8000/app/userSettings

This approach is used by Twitter (create a tweet) and Coinbase (see a transaction detail)

Solution

Deciding if a modal should update the URL or not is a tricky decision with pros and cons. Everyone does it differently because it depends.

In my opinion, there are modals where it makes sense to update the URL (ex: see payment details), and others where it does not make sense (e.g. See all members in a group).

I think our solution should allow both approaches taking into account the content/context of each modal. (with updating the URL as default)

And when opening a modal inside/after another modal, the URL should only reflect the most recent opened modal.

---

Update (9/12/2020): Followup to #946

946 was merged with a pending PR review since this issue is being re-assigned to @sdegutis. To close this issue please open another PR to address the issues found in that review, specifically:

1. Missing call to remove listener / or better yet, removing the SET_MODAL_QUERIES event altogether
2. Using router guards to remove code duplication resulting from SET_MODAL_QUERIES

[sandrina-p]

@taoeffect, I'd like to hear your thoughts on this.

While doing #893, I'm facing again the same question. We have the modal to change the voting rule. If we open the modal using props, that would cause the modal to be lost if the page is refreshed. If we use queries, everything would work fine.

// this link is useless to open the modal
http://localhost:8000/app/group-settings?modal=ChangeVotingRules

// this link works correctly
http://localhost:8000/app/group-settings?modal=ChangeVotingRules&rule=disagreement

At the moment we only have 2 modals that rely on "props". Doing the refactor to pass the data by query instead of props is a low effort and easy to do.

[taoeffect]

@sandrina-p OK go ahead and refactor it to work with query. That seems reasonable to me.

Regarding "a modal as a query" vs "a modal as a route", am definitely in favor of our current approach of "a modal as a query" as well, since that lets us specify the page that is shown when the modal is closed.

[sandrina-p]

Okay, I'll open a PR where modals use queries instead of props.

I agree with using "a modal as a query" as well.

What about <button> vs <a> ? What are your thoughts?

[taoeffect]

What about <button> vs <a> ? What are your thoughts?

I agree with your @sandrina-p, that solution with <router-link> looks much nicer since it doesn't involve having to create a function in the component (which we've been doing so much that it amounts to a DRY violation). My only concern though is to make sure that this approach doesn't break any existing stuff related to accessibility or using the enter key, etc. But you would be the expert on that!

[sandrina-p]

For A11Y, there isn't any disadvantage in using <a/>. In fact, there's one advantage as I mentioned before, the right-click to open the modal in a new tab.

[sdegutis]

Also Shift + Enter when <a> is focused will open it in a new tab, but won't work for <button>.