Problem/Motivation

Implement the new designs for the Mobile Toolbar.

Proposed resolution

Figma link: https://www.figma.com/file/fn52micITi1fCNtU0fWcPC/Prototype?type=design&...

CommentFileSizeAuthor
#21 RPReplay_Final1696865423.MP43.73 MBlauriii
#14 Screenshot_20231005-120744.png108.84 KBclaireristow
#2 mobile.png335.17 KBckrina

Issue fork navigation-3384996

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ckrina created an issue. See original summary.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
StatusFileSize
new mobile.png335.17 KB
claireristow’s picture

claireristow commented
Assigned: Unassigned » claireristow

I really like this solution Cristina! Working on this now.

ckrina opened merge request !63

claireristow’s picture

claireristow commented
Assigned: claireristow » Unassigned
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

Just noting in here (because it was something we just discussed live with @claireristow) that what I pushed to the MR was just an initial proof of concept for her to see what I had been plying with, and it isn't usable. So whoever takes this on: please better start from scratch with a new MR :)

finnsky’s picture

finnsky commented

@ckrina btw which is breakpoint between mobile desktop? 1024?

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Title: Mobile Toolbar implementation » [PP1] Mobile Toolbar implementation
Status: Active » Postponed
Related issues: +#3386927: [PLAN] Refactor CSS

@finnsky I assumed 35rem for now. It might be subject to change if we see another size works better once this is evolves.

Postponing this issue until #3386927: [PLAN] Refactor CSS gets done to be sure there aren't too many conflicts.

finnsky’s picture

finnsky commented

we maybe need 2 breakpoints:

- one is about 35rem=560px before it toolbar works as mobile.
- another is about 1024px, between 560-1024 toolbar will work with expand collapse but will NOT shift content.
- 1024+ desktop behaviour

finnsky’s picture

finnsky commented
Status: Postponed » Active

finnsky opened merge request !78

finnsky’s picture

finnsky commented
Status: Active » Needs review

What added:

1. js/admin-toolbar-wrapper.js: {}

Js for managing toolbar triggers and wrapper behaviour.
Also controls html event listeners and Drupal.displace.

Existing sidebar.js contains only inner Sidebar functions.
Later should be also splitted to:
- details(or whatever we name it).
- tooltip functions.
- menu functions.
- etc.

2. Media queries. For now only one 1024. Because we cannot start layout shift on 560.

https://gyazo.com/7edd48da28db2270af19b6ae1294ec5e

I have suggestion how to rework it
* 0-559 works as explained on mobile design.
* 560 - 1023 works as desktop without displace shift.
* 1024+ works as desktop.

3. Added `js-` prefixes to html toogled classes. Maybe it will helps some developer not use them in twig.

4. Added `js-admin-toolbar-activated` helper class to avoid layout shifts and text change shifts.

5. Added `control-bar` css component. it has temporary styled button. Hidden on 1024+. Maybe should be fixed on top. Not clean in design.

6. Added `admin-toolbar__close-button` also styled temporary. Hidden on 1024+

7. Toolbar always collapsed on load on mobile.

8. Sidebar.js and all css definetly need some style/eslinting. Tryed to avoid in this MR.

finnsky’s picture

finnsky commented

Some minimal regressions in spacings and floating tooltip behaviour.
Should be managed in follow up tickets.

claireristow’s picture

claireristow commented
StatusFileSize
new Screenshot_20231005-120744.png108.84 KB

This is looking so great @finnky!! Just noticed these two small things which can be seen in my screenshot as well:

  1. The second level dropdown arrows have lost their right padding so they're showing tight to the background color.
  2. In the designs, it looks like we're hiding the "collapse/expand sidebar" item in the sticky menu in favour of the close button in the top right corner of the sidebar. Is that still accurate or did we decide to keep it?
finnsky’s picture

finnsky commented

Thanks for review.
1. Should be resolved in follow up issues i guess. I hope parts of this will merged https://www.drupal.org/project/navigation/issues/3391533
2. I cannot understand it from design, can be follow up with disqussion if we need it. Can be easy hidden for sure.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
Status: Needs review » Needs work
Related issues: +#3392452: Toolbar Prototype Usability Testing Phase 3

Answering more things per parts:

About the breadcrumb: great idea! But let's keep issues focused on the main purpose defined and open extra things big enough detected in follows-ups. I’ve created [3392481] to deal with the breadcrumb work. Feel free to assign it to yourself and add the work in there.

In the designs, it looks like we're hiding the "collapse/expand sidebar" item in the sticky menu in favour of the close button in the top right corner of the sidebar.

This is the desired behavior. We don't want the neither bottom sticky region nor the "Collapse sidebar" button on mobile. And that region needs to be listed right below the last menu from the medium region as if it was another element in the flex region.

1. js/admin-toolbar-wrapper.js: {}

Really cool that you’ve broken it into different files!

2. Media queries. For now only one 1024. Because we cannot start layout shift on 560.

Agreed, 560px doesn’t work. 1024px seems fine for now, and we can change later if we want thanks to defining it in one place.

I have suggestion how to rework it (…) * 560 - 1023 works as desktop without displace shift.

One of the feedback we got from the initial round of user tests was to not have a middle step the current toolbar has happening without notice. Even though what you are suggesting is not exactly what the current Toolbar is doing, I think this idea could use some user tests before implementing. I’ve added it to #3392452: Toolbar Prototype Usability Testing Phase 3 so we can test during DrupalCon, but it’ll probably need a MR to do A/B testing. So let’s open a follow-up to implement it when this is merged.

5. Added `control-bar` css component. it has temporary styled button. Hidden on 1024+. Maybe should be fixed on top. Not clean in design

.
I've added a link to Figma in the summary because it might help as a reference, but specific things like spacing and colors (or even the borders) might change when we get the final designs.

8. Sidebar.js and all css definetly need some style/eslinting. Tryed to avoid in this MR.

Cool, feel free to open a new issue when this is merged in to handle all that.

Again, great work!!

finnsky’s picture

finnsky commented
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Title: [PP1] Mobile Toolbar implementation » Mobile Toolbar implementation

Removing the [PP1] tag from the title that I missed.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Needs work

Missed changing it to NW for the latest small feedback I found.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new RPReplay_Final1696865423.MP43.73 MB

There's some horizontal scroll within the navigation itself at least on iPhone which is pretty annoying. Is that something we should address here or in a follow-up?

finnsky’s picture

finnsky commented

RE #21

I think follow-up needed, I don't have this in android. Also we need to use
dvh here. Because on my side i have `User link` outside of screen sometimes.

https://caniuse.com/mdn-css_types_length_viewport_percentage_units_dynamic

Upd. Views core js also breaks toolbar on mobile

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

  • ckrina committed 5b92e180 on 1.x authored by finnsky 
    Issue #3384996: [PP1] Mobile Toolbar implementation
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Fixed

Merged! 🎉

Thanks @finnsky for all the work here.

Let's move everything we've found into follow-ups so it's easier to handle them.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.