Start making changes to the onboarding flow #1189

joshsmith · 2017-03-24T04:25:25Z

What's in this PR?

This is a work in progress to make changes to the onboarding flow when donating.

References

joshsmith · 2017-03-24T04:58:57Z

@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

@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

begedin · 2017-03-24T14:18:54Z

app/mixins/donation-route.js

+      this._super.call(...arguments);
+    }
+  }
+});


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

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

begedin · 2017-03-25T17:09:44Z

app/controllers/application.js


+  _updateRoute: observer('currentRouteName', function() {
+    get(this, 'currentRoute').didTransition();
+  }),


I can't say it's clear to me what this part does. I mean, I get the what, not clear on the why

begedin · 2017-03-25T17:10:47Z

app/mixins/donation-route.js

+
+  setupController(controller, models) {
+    controller.setProperties(models);
+  }


I would destructive the hash here, so it's clearer what the models are

begedin · 2017-03-25T17:16:53Z

app/routes/project/donate.js

+        let subscription = get(this, 'userSubscriptions').fetchForProject(project);
+        return RSVP.hash({ project, subscription });
+      } else {
+        return RSVP.hash({ project, subscription: null });


Do we use this behavior anywhere else? If so, maybe fetchForProject should just return null if no current user.

It looks like it already does...hm.

begedin · 2017-03-25T17:19:37Z

app/templates/project/donate.hbs

@@ -1,5 +1,6 @@
 <div class="container">
  {{donation/project-header project=project}}
+  {{log project}}


This one is easily left behind, so just a note here to remove it.

Pushed up fixes for everything mentioned

joshsmith · 2017-03-27T02:52:48Z

This very badly needs tests. I think we need to test that:

You can advance forward and backward through the various onboarding states regardless of whether you're currently onboarding
The onboarding header types show up when you're onboarding
Clicking the logo in the header when signing up to donate will take you into the onboarding
The user state is not set is not transitioned in weird ways
Any various unit testing we can do

joshsmith · 2017-03-27T02:58:04Z

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

joshsmith · 2017-03-27T03:01:50Z

I also included the URI in the browser so that you could see where exactly we're navigating.

begedin · 2017-03-27T14:30:30Z

app/services/current-route.js

+  updateCurrentRouteName() {
+    let { currentPath } = getOwner(this).lookup('controller:application');
+    set(this, 'currentRouteName', currentPath);
+  }


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')

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.

Yes, and I think it's relatively small exposure of its API surface area.

joshsmith force-pushed the add-onboarding-improvements branch from 732362e to 9d74984 Compare March 24, 2017 04:27

joshsmith added the needs work label Mar 24, 2017

joshsmith assigned begedin Mar 24, 2017

begedin reviewed Mar 24, 2017

View reviewed changes

begedin reviewed Mar 25, 2017

View reviewed changes

begedin previously requested changes Mar 25, 2017

View reviewed changes

joshsmith added needs code review and removed needs work labels Mar 27, 2017

begedin force-pushed the add-onboarding-improvements branch from e885748 to b1a3147 Compare March 27, 2017 07:37

begedin reviewed Mar 27, 2017

View reviewed changes

Make the onboarding flow more flexible

9fc7956

joshsmith force-pushed the add-onboarding-improvements branch from 0fbb768 to 9fc7956 Compare March 27, 2017 17:52

joshsmith added ready to merge and removed needs code review labels Mar 27, 2017

begedin approved these changes Mar 27, 2017

View reviewed changes

joshsmith merged commit 0687f57 into develop Mar 27, 2017

joshsmith deleted the add-onboarding-improvements branch March 27, 2017 17:59

Start making changes to the onboarding flow #1189

Start making changes to the onboarding flow #1189

Uh oh!

Conversation

joshsmith commented Mar 24, 2017

What's in this PR?

References

Uh oh!

joshsmith commented Mar 24, 2017

Uh oh!

begedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshsmith commented Mar 27, 2017

Uh oh!

joshsmith commented Mar 27, 2017

Uh oh!

joshsmith commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants