UI Onboarding Wizards #5196
UI Onboarding Wizards #5196
Conversation
…e unseal description string
… add auth event for idle
Hide UI Guide on mobile
Mobile styles for UI Guide
Text updates for UI Guide
There was a problem hiding this comment.
Review Pt 1: The JavaScript
I read and reviewed everything up to the templates. I skimmed through the CSS assuming it had already been reviewed and merged into this branch.
This looks like mondo excellent work. I saw some places for developer experience improvements, but probably nothing that hasn't already been thought of and passed over for the sake of time.
Can't wait to see how this evolves! I imagine something like this will end up in all products, so this is likely to be the beginning of something that we all share.
| size: computed(function() { | ||
| return 12; | ||
| size: computed('glyph', function() { | ||
| return this.get('glyph').startsWith('enable/') ? 48 : 12; |
There was a problem hiding this comment.
This strikes me as surprising. I wouldn't expect certain icons to get special treatment implicitly.
| this.get('wizard.featureState'), | ||
| 'CONTINUE', | ||
| this.get('mountModel').get('type') | ||
| ); |
There was a problem hiding this comment.
Does it make sense to make a special method for this pattern? Something like...
// wizard.js
next(extendedState) {
return this.transitionFeatureMachine(this.get('featureState'), 'CONTINUE', extendedState);
}
// this-file.js
try {
if (config && Object.keys(config.changedAttributes()).length) {
yield config.save()
this.get('wizard').next(this.get('mountModel').get('type'));
}
} catch (err) {
// ...
}| this.get('wizard').transitionFeatureMachine( | ||
| this.get('wizard.featureState'), | ||
| 'RESET', | ||
| this.get('mountModel').get('type') |
There was a problem hiding this comment.
Another one! Maybe it also makes sense to "curry" this for two arguments as well as three and one?
// wizard.js
// Total control
transitionFeatureMachine(currentState, event, extendedState) {},
// Assume internally tracked state
transition(event, extendedState) {
this.transitionFeatureMachine(this.get('wizard.featureState'), event, extendedState);
},
// Convenience methods for common event types
next(extendedState) { this.transition('CONTINUE', extendedState); },
reset(extendedState) { this.transition('RESET', extendedState); },Just looking for ways to reduce boilerplate and abstract away some internals.
| @@ -0,0 +1,11 @@ | |||
| // THIS COMPONENT IS ONLY FOR EXTENDING | |||
| // You should use this component if you want to use outerHTML symantics | |||
| }); | ||
|
|
||
| // yep! that's it, it's more of a way to keep track of what components | ||
| // use tagless semantics to make the upgrade to glimmer components easier |
| saveFeatures() { | ||
| let wizard = this.get('wizard'); | ||
| wizard.saveFeatures(this.get('selectedFeatures')); | ||
| wizard.transitionTutorialMachine('active.select', 'CONTINUE'); |
There was a problem hiding this comment.
Super vague thinking here... would it make sense to split wizard into two services, one for each FSM?
|
|
||
| export function engines() { | ||
| return MOUNTABLE_SECRET_ENGINES; | ||
| } |
There was a problem hiding this comment.
Since this function is just returning a a reference to the MOUNTABLE_SECRET_ENGINES set, you might as well just export the const instead of a function. Either that or use copy(MOUNTABLE_SECRET_ENGINES, true) to make sure no consumer of the export mutates it.
| onEntry: ['completeFeature'], | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
I still love how declarative these machines end up. In my head it goes
- This PR
- ???
- Hook it up to a CMS
- ???
- Profit
| ], | ||
| on: { | ||
| CONTINUE: { | ||
| details: { cond: type => supportedBackends.includes(type) }, |
There was a problem hiding this comment.
These cond predicates are cool even though they step on my CMS dream.
| storage() { | ||
| return getStorage(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Some of these methods are rather dense, but I'm not sure if they need more comments or if I just need to know more about xstate.
* initial try at pause * save resume url when pausing and replay machines when resuming * don't save resumeURL if it's not set * don't need currentURL in execute actions * re-set the state on resume * fix tools not triggering pause * small fixes for pausing * fix policies machine
No description provided.