Repository navigation
Start making changes to the onboarding flow - #1189
Conversation
732362e to
9d74984
Compare
|
@begedin I have scattered thoughts around the issues above here, but combined with the progress made here and #1098 especially, you should be able to get the outline of direction I'd ideally like to see. Hopefully you can provide some thoughts/notes at least on how to proceed, so I can maybe wrap this up tomorrow morning. |
begedin
left a comment
begedin
left a comment
There was a problem hiding this comment.
@joshsmith If I'm reading this correctly, if a user signs up for donating then
- from our previous PR, they will be immediately redirected back to the donation page they were at after signing up.
- if they quit the browser, or do some other thing, when they get back, they will be allowed to the 3 donate routes, the terms and privacy routes
- if they visit any other route, they will get redirected to the typical onboarding route (so the editing profile one).
Is that correct? If so, it makes sense and is really probably the simplest thing we can do to push out a done feature. If it requires further complexity, we can continue work on it through other issues/PRs
| this._super.call(...arguments); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
This mixin works, and is probably the next best thing, but combined with the fact that these routes have an explicitly separate layout to other project routes, it's really further telling us that our onboarding routes ought to be grouped under a parent route of some sort. That would allow us to have this afterModel as well as our model within that parent route.
It's not something suitable for this route, but it is something to consider and probably discuss.
begedin
left a comment
begedin
left a comment
There was a problem hiding this comment.
Just a quick note. The mixing and the services need a sort of moduledoc explaining what they do. The naming implies overlap and does not make it clear. I can figure out what they do if I apply myself, but it takes a bit to get onboarded, pun intended. I'm afraid of what will happen when we at some point get back to it for some reason
|
|
||
| _updateRoute: observer('currentRouteName', function() { | ||
| get(this, 'currentRoute').didTransition(); | ||
| }), |
There was a problem hiding this comment.
I can't say it's clear to me what this part does. I mean, I get the what, not clear on the why
|
|
||
| setupController(controller, models) { | ||
| controller.setProperties(models); | ||
| } |
There was a problem hiding this comment.
I would destructive the hash here, so it's clearer what the models are
| let subscription = get(this, 'userSubscriptions').fetchForProject(project); | ||
| return RSVP.hash({ project, subscription }); | ||
| } else { | ||
| return RSVP.hash({ project, subscription: null }); |
There was a problem hiding this comment.
Do we use this behavior anywhere else? If so, maybe fetchForProject should just return null if no current user.
There was a problem hiding this comment.
It looks like it already does...hm.
| @@ -1,5 +1,6 @@ | |||
| <div class="container"> | |||
| {{donation/project-header project=project}} | |||
| {{log project}} | |||
There was a problem hiding this comment.
This one is easily left behind, so just a note here to remove it.
Pushed up fixes for everything mentioned
|
This very badly needs tests. I think we need to test that:
|
|
I've uploaded a demo that shows all the behavior as I would expect it to work, since this is simpler than writing it out: https://cl.ly/2c0M3Q3g0z27 |
|
I also included the URI in the browser so that you could see where exactly we're navigating. |
e885748 to
b1a3147
Compare
| updateCurrentRouteName() { | ||
| let { currentPath } = getOwner(this).lookup('controller:application'); | ||
| set(this, 'currentRouteName', currentPath); | ||
| } |
There was a problem hiding this comment.
This works fine, but it's a bit of a confusing way to just set a property on an object, which this service really is. It's an object, with a single property, which is a singleton in the context of our application. I'm thinking, why not just have the routes that call get(this, 'currentRoute').updateCurrentRouteName() instead just call something like
set(this, 'currentRoute.currentRouteName', 'some.new.name');While we're at it, since the name of the service is already currentRoute, why not call the property currentRoute.name, instead of the redundant currentRoute.currentRouteName.
Lastly, and this is a big one, there is a router service in ember, though it's still private right now:
It's deinfed here and is used by the LinkComponent here.
Basically, we can replace the usage of our own currentRoute service by simply replacing injections of it with
routing: service('-routing'),We can name it currentRoute to, if we want.
We can also then alias the currentRouteName easily:
currentRouteName: alias('router.currentRouteName')
There was a problem hiding this comment.
Also note that while I do understand any concerns with this service being marked as private right now, there have been RFCs about opening it up, the documentation itself states it, and it seems to be a big part in making components routable, so I would not be too concerned about it.
There was a problem hiding this comment.
Yes, and I think it's relatively small exposure of its API surface area.
0fbb768 to
9fc7956
Compare
What's in this PR?
This is a work in progress to make changes to the onboarding flow when donating.
References
Fixes #1094
Fixes #1097
Fixes #1098