Skip to content
This repository has been archived by the owner on Mar 5, 2020. It is now read-only.

Show add club modal on page load if ?modal=add is in url on clubs page. #567

Merged
merged 1 commit into from
Apr 4, 2015

Conversation

toolness
Copy link
Contributor

@toolness toolness commented Apr 2, 2015

This fixes #566.

Note that it doesn't remove ?modal=add when the modal is dismissed. This is partly because it's a hassle to implement and test, but also partly because I'd like to implement a more robust solution for giving every modal its own URL in v2, which I allude to a bit in #373 (comment) (this will make it a lot easier to allow older/non-JS browsers to use the modals too, among other things). As such, this PR is really just a quick fix to unblock mozilla/id.webmaker.org#51.

@ScottDowne
Copy link
Contributor

R+ ✅ ✅ ✅

We could use this for the "add club" link at the bottom of the club toolkit page.

I gave it a quick look but it looks like it needs to use the Link component and that's not a simple linkTo += "?modal=add"so I say punt it to a new issue.

ScottDowne added a commit that referenced this pull request Apr 4, 2015
Show add club modal on page load if ?modal=add is in url on clubs page.
@ScottDowne ScottDowne merged commit 0629c9f into mozilla:develop Apr 4, 2015
@toolness
Copy link
Contributor Author

toolness commented Apr 4, 2015

Thanks Scott!

I gave it a quick look but it looks like it needs to use the Link component and that's not a simple linkTo += "?modal=add"so I say punt it to a new issue.

Yeah, in v2 I'd actually like to change all modals have their own routes, e.g. /clubs/add for the clubs modal; I explain this a bit more in #373 (comment) but the gist is that it'll make react-router do more of the heavy lifting for us, it'll make the code more "react-esque" and easy to reason about, and it'll make it possible to make the modals accessible for folks without JS.

For now though I think it's best to keep ?modal=add's usage limited to a "hack" that allows our login flow to work--for instance, just turning the "add your club" into a <Link> won't actually work with this PR because that JS is only triggered in componentDidMount--meaning that clicking on the Link won't actually trigger the modal, because the component is already mounted! And I don't think that'd be the last of our problems, either... Giving every modal its own route in v2 will totally fix this, though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants