-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update/gutenboarding hide next button in intent gathering step #38705
Update/gutenboarding hide next button in intent gathering step #38705
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~30923 bytes added 📈 [gzipped])
Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~1309870 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~5685658 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~2590240 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Moment.js Locales (~108564 bytes added 📈 [gzipped])
Locale data for moment.js. Unless you are upgrading the moment.js library, changes in these chunks are suspicious. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Would be good to get a 👍 from @Automattic/luna too to check for any gotchas
Thank you both for reviewing! @roo2 I was thinking the same thing, but thought I'd just keep the PR to the Next button, in case folks from Luna were testing out things in the sidebar / block settings? |
Yep, an issue here: #38686 |
<Link to={ next } className="gutenboarding__header-next-button" isPrimary isLarge> | ||
{ NO__( 'Next' ) } | ||
</Link> | ||
{ ! isNextHidden && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we change this to
{ ! isNextHidden && ( | |
{ ! next && ( |
?
Falsey next
can indicate that there's no next step to go to, and it'll allow us to drop the extra bool prop.
(I'm generally a bit wary of bool props controlling visibility of elements, since we might easily end up with an unwieldy number of them, especially in a context like Gutenboarding, which is all about a 'progressive chrome' that adds more and more UI elements as the user progresses through the steps of the onboarding block.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ockham thanks I wasn't too sure about adding the extra prop. In your suggestion, I think you mean we'd check that next
is truthy to render the button?
The only problem is that checking if next
is truthy will hide the disabled next button on the design step, too. Another way we could do it would be to set the value of next
to a constant like 'HIDE_BUTTON'
and if that's the value, then hide the button. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented it that way at first in e548cae but then second guessed myself :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only problem is that checking if next is truthy will hide the disabled next button on the design step, too.
I'm a bit unsure of myself, but I'll ask anyhow. Under the current architecture, wouldn't the design step define its own next
in the switch block (thereby showing the next button)?
case Step.IntentGathering:
break;
case Step. DesignSelection:
next = Step. SomeFabulousStep;
break;
Or is it more to do with the disabled state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think it would. I'm possibly over thinking it, but I was mostly thinking about how do we want to handle the disabled state. So we've got 3 potential states for the button: hidden, disabled, and active link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we've got 3 potential states for the button: hidden, disabled, and active link.
Got it. I suppose we'll want to update the button based on validating input values in the main section or some such thing in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ockham thanks I wasn't too sure about adding the extra prop. In your suggestion, I think you mean we'd check that
next
is truthy to render the button?
Yeah, you're right 😅
The only problem is that checking if
next
is truthy will hide the disabled next button on the design step, too.
That's a good point.
Another way we could do it would be to set the value of
next
to a constant like'HIDE_BUTTON'
and if that's the value, then hide the button. What do you think?
Let's avoid magic constants 😬 They're a bit too obscure, and go a bit against the whole types paradigm.
It's fine to go with either the current state of the PR (that checks next
), or to revert to your original isNextHidden
approach. My apologies for delaying this PR, we probably shouldn't overthink it at this stage.
Just to explain why I was skeptical about isNextHidden
-- it's mostly about how this pattern will scale:
One of Gutenboarding's paradigms is a 'progressive chrome' -- the editor (with its header and possibly sidebar) adds more and more UI controls, as the user progresses through the onboarding steps in the onboarding block.
With the isSomethingHidden
bool pattern, we'd end up with a number of bool props that control which of those UI elements to conditionally hide, depending on which step we're currently at. This is exacerbated by the fact that those UI controls might dynamically change (e.g. the 'Next' button label might be different at different stages).
I don't have a clear answer how to best solve this right now, but I'm leaning towards 'regions' components that accept a number of component props to be placed in different parts of the UI. See e.g. https://github.com/WordPress/gutenberg/blob/a8f43fa7e21a028b43d0cb023ac5c872a3fbd4d8/packages/edit-post/src/components/editor-regions/index.js (introduced by WordPress/gutenberg#18044, which has some explanation in its PR desc).
One problem that's a bit different for our use case is encapsulation -- I don't think we want gutenboarding.tsx
to pass all those granular components down to e.g. the header or sidebar; instead, those 'container'/'region' components should infer themselves which components to display at what stage. Some of that information can be inferred from the onboarding data store; we'll still have to consider how to make those components aware of routing, tho (maybe we'll end up moving our main routing logic out of the onboarding block -- I don't know at this point). Note that this is basically a question about which pattern we'll use to build the 'custom editor'.
Anyway, I won't block this PR any longer 😅 Let's just keep this in mind as we add more 'conditional' UI elements.
Just realized we'll need to hide the button for now also at the "design picker" step and show it only at "page selector" step. That's the only step in the flow actually when the button is visible: The label has to be "Create my site" at the last step. (designs via paObgF-RB-p2 by @dwolfe ) Does that help thinking how to implement this? |
This is ready for another review — apologies if I've again over thought things! Because the button should only appear after a user has selected their design, I've gone ahead and moved the |
Also, looks like the page layout picker will be moved to the onboard store in #38757, so hopefully this change should play nicely alongside that one, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, much of the changes look good. The scope grew a bit beyond the original intention, but I think it's a reasonable size and set of changes 👍
I've left some comments. I do have concerns about putting more routing logic into the header, which spreads the routing logic out more. I'm not sure what the answer is. I'd like to keep the routing logic colocated, a dedicated module or a selector on the onboarding store seem like viable options. I'd like to yours and other's thoughts.
const selectedDesign: Reducer< | ||
import('@automattic/data-stores').VerticalsTemplates.Template | undefined, | ||
ReturnType< typeof Actions[ 'setSelectedDesign' ] > | ||
> = ( state = undefined, action ) => { | ||
if ( action.type === ActionType.SET_SELECTED_DESIGN ) { | ||
return action.selectedDesign; | ||
} | ||
return state; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good. Did you have any hangups working on this?
export const setSelectedDesign = ( | ||
selectedDesign: import('@automattic/data-stores').VerticalsTemplates.Template | undefined | ||
) => ( { | ||
type: ActionType.SET_SELECTED_DESIGN as const, | ||
selectedDesign, | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
client/landing/gutenboarding/onboarding-block/design-selector/index.tsx
Outdated
Show resolved
Hide resolved
client/landing/gutenboarding/onboarding-block/design-selector/index.tsx
Outdated
Show resolved
Hide resolved
<Link to={ next } className="gutenboarding__header-next-button" isPrimary isLarge> | ||
{ NO__( 'Next' ) } | ||
</Link> | ||
{ next && hasSelectedDesign && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of making the header more aware of the component and step logic and selecting more state. I'm not sure what the solution is right now 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me neither, at this point. I think we'll reconsider as Gutenboarding continues to evolve. Also see my (lengthy 😅 ) comment here: #38705 (comment)
The header is going to be evolving quite a bit in next weeks;
So I think it's safe to have some less-optimal code now in, and we'll just have to make sure to come back to improve it if we don't end up removing it. I don't wanna undermine the work here by saying that this might be removed in future iterations, but it's kinda just the reality of Gutenboarding-prototype work currently; some things are more certain and some things less so. Hope this helps! |
Sorry to bloat this PR's comment thread, it's probably best elaborate elsewhere, but for the record, we had some interesting debate about the role of a "synthetic" back button and alternatives back in the day. At the core was the confusion brought about not only by duplicating the back button, but by the cases in which the back buttons (i.e., the browser's and ours) behaved differently, or did not do what the user expected them to do. Towards the end, though no development tasks arose, an agreement percolated to the effect that any synthetic back button, or progress navigation we build into the flow should explicitly communicate to the user what is going to happen when they click on it. See: Other related issues/PRs |
cef3b85
to
39b9ba3
Compare
Thanks very much for the reviews and detailed thoughts everyone, it's very useful as I gradually form a mental model of what we're trying to achieve with Gutenboarding (and learn TypeScript in the process). I've implemented the suggestions so far, and left the question of what to do about state-aware headers for another time. Let me know if there are any other changes you'd like, though!
This sounds good to me — particularly while we're pre-launch with this flow, I'm more than happy to circle back on cleanup tasks as we go. Changes like in this PR are useful learning tools, so even if the code ends up being discarded it's still a good process to go through. Thanks @ockham for going into greater detail about the "progressive chrome" idea and the scalability concerns of patterns in this part of the codebase. The regions idea does sound promising. @sirreal I've worked through those suggested changes, thanks for the detailed look. Overall, working with TS is really growing on me, and having existing patterns and types in the codebase to refer to (like |
If anyone gets a chance, this one's ready for another review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as advertised. Just curious, if this being a next-step-trigger after selecting a design, should it also be available after selecting a design and collapsing the page selector? See:
Although I notice that going back/forth a step (to intent gathering), the design selection does not persist. So there may be some room to improve the flow there a little (?) e.g. keep the design selection throughout the session? If not, then the lingering selection when page-selector is collapsed currently doesn't serve much input (other than indicating what design was clicked previously).
39b9ba3
to
c27b14e
Compare
…press/data store instead of React useState hook
…or, and minor cleanup
a3d690e
to
1e3bc27
Compare
Thanks for reviewing @chriskmnds!
That's a good question! Currently the
I think the lingering focus state on the selected design after hiding the page selector is probably fine. If you're using the keyboard to navigate, after hitting escape it means you don't lose your place in the list of designs. I'll leave this for now to contain the scope of this PR, but it's a good point about holding on to the selected design state when a user navigates back to the intent gathering step. Let's circle back to this separately. |
Just one point of clarification: the design picker and the page selector are intended as sequential steps, so there's actually no "page selector collapsed" state. The only reason to collapse the page selector is to select a different design, which I indicated on the first iteration as a "Choose a different design" link. |
Thanks for confirming @dwolfe! |
Makes sense, although I feel that's more like a secondary (or advanced) interaction. Seeing that blue box, my mind goes to "i have a design selected and pages within it (or some default empty). I can move on". Quick thoughts:
|
Changes proposed in this Pull Request
setSelectedDesign
behaviour to use the onboardwordpress/data
store instead of React state hook.Before
After
Testing instructions
http://calypso.localhost:3000/gutenboarding
and in the top right hand corner there shouldn't be a Next button/gutenboarding/design
step, after you select a design, you should see the Create my site button at the top rightFixes #38698