-
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
Store: Clean up header on smaller screens #22541
Conversation
Screenshots look fab! Eagerly awaiting the designer feedback on this one. Also /cc @shaunandrews for any thoughts on how smaller viewports might handle the new sidebar navigation concepts... maybe he has some designs around this already too. |
I don't have anything for this yet — we'll likely be using the SidebarNavigation for a while longer. At some point, we may combine it with the sidebar rather than having it as a separate component people need to remember to include. |
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.
Overall i'm really like how this is working/looking - nice job @ryelle!
I did notice a few minor oddities. The first being when editing an existing product - there is a range between breakpoints where the action button, in this case 'Update' is cut off and off screen. Also note the redundant display of the fact you are Editing a Product:
Here is another instance where we show the current page/tab action 3x - seems a bit overkill:
I did browse through all views - orders, products, promotions, reviews, and settings and everything was looking quite nice.
Nice idea! I agree with Timmy about the repetition, and I think there's some value in the breadcrumbs so I'd like to see us keep them if possible. How about something that combines the smaller site header / crumbs? Then keep the primary action button in the header and have secondary actions 1 click away via an ellipsis button. |
Trying this out – which means totally refactoring how the I'll have to adjust how it looks on larger screens, and I'll switch the actions to a SplitButton. I'll also try to add some smart space detection like the |
Looks good, nice work! :) I know I'm splitting hairs and it might be more trouble than it's worth, but...
Could just be
I know we might end up with wrapping on products with long names but it'd be nice if we could strive to avoid it. We might also consider putting a max-width in the crumb trail and fading the overflow to totally rule out wrapping as an alternative. |
That is looking fantastic @ryelle 😍 |
bf39fd8
to
72a5d32
Compare
I think the split button works quite nicely here - but will defer to @kellychoffman / @jameskoster 😁 |
Looks amazing! Agree we should fix those other issues in separate PRs. |
…so to hide when there are no actions (when this is the case, the header is redundant to the site nav header)
…redo it in each screen
@kellychoffman @jameskoster @shaunandrews @timmyc – I don't think I got a final 👍 / 👎 on whether this should be merged, which is partially my fault since I didn't flip it back to needs review. Knowing that it's not perfect, should this be merged as-is? |
7a9f27e
to
fbb79fd
Compare
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.
Took it for another spin, looks great.
Since it's not a single line/single file change, I'd rather see it in a different PR. That also wouldn't fix the issue in the |
Includes fixes for this new toolbar view: Automattic/wp-calypso#22541 To test `env BROWSERSIZE=desktop ./node_modules/.bin/mocha specs-woocommerce` `env BROWSERSIZE=mobile ./node_modules/.bin/mocha specs-woocommerce`
Includes fixes for this new toolbar view: Automattic/wp-calypso#22541 To test `env BROWSERSIZE=desktop ./node_modules/.bin/mocha specs-woocommerce` `env BROWSERSIZE=mobile ./node_modules/.bin/mocha specs-woocommerce`
This PR is just an idea – rather than drop down to icons-only for the action header on smaller screens, or a split button (which we would have to use for all sizes), why not bring back the small site header, and conditionally drop the breadcrumbs?
There are more screenshots in this gallery, some are at 320px wide, and some are around 430px.
Technical details
I've unhidden the SidebarNavigation (that top section with the site icon, name, and section title) on the store screens, and fixed an issue where it was being called twice on a few screens. This also updates ActionHeader to add classes when there are action buttons, and specifically when there are multiple actions (which requires the buttons to be passed as an array).
On screens smaller than 480px wide, if the
ActionHeader
has…There is an issue with the section title not updating correctly, I have another PR for that #22501
This was just an idea I had while trying out the orders mobile styling from #17910 – the < from the SidebarNavigation was acting janky (it stayed in place while the page scrolled), so I went to fix that & found this entire component 🙂