Barcelona
Account created on 7 March 2011, almost 13 years ago
#

Recent comments

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

More issues added to define the plan better.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Tested and reviewed the code, so moving this to RTBC. Also removing the NFMR tag so it can be committed. Thanks all!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

ckrina β†’ created an issue.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

This has been addressed already and is working in 1.x. Thanks @christophdubach for taking the time to post this!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thanks all for the work and help! Merged :D

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Adding some JS related issues in the issue summary, on the Should have for now.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

We just discussed this with @finnsky and it looks like everything on the summary has been addressed so far:

In the JS all the element selectors should rely on data attributes and not class names, things such as .toolbar-menu__item.toolbar-menu__item--level-2 etc.

This has been done.

There are some syntax issues, ifs that are not wrapped with {}, multiple declarations of variables on one line. essentially the eslint core config should be run on the code and fixed.

I don't have a good solution for that yet, the code could use a refactor to improve readability possibly grouping the positioning into one file/area of the code that sort of things.

After componentizing the CSS and JS and breaking the JS in several files ("no monolithic js anymore" per @finnsky words) we think this is addressed too. The work happened in:

Maybe this would need another round to review what needs to happen for core, or otherwise we could close this one.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thanks all for your work here! After testing this I've found an usability problem this is introducing: if it scrolls too much to fit as much of the item on the screen the 1st menu item label is lost. This label is what actually gives you the context to know which menu item you're in, and if it gets moved beyond the visible area that context is lost.

The ideal behavior would be to place the whole submenu on the visual space, but always ensuring the label of the parent menu item being opened is visible. That could be done in several ways, like calculating its height in the current implementation or rethinking if the current approach of showing "the nearest" item is the best one.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

What I meant in #5 is if it would it work to do something like this:

:where([data-admin-ui-initial-styles]) * {
    font-family: inherit;
}

This way this would be defined in the admin-initial-styles.css for all components without the need to specify it on each component file.

If that approach works it'd mean to postpone this issue until ✨ Reset theme css. Needs work is in and this gets integrated in that file.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thank you @ldonelan! I've updated the links to use the pattern for issue numbers so the state gets pulled automatically.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Posting this here also to show that I've answered (as all the other times this came up in Slack):

As I mentioned in the past I don’t have a strong opinion on this, specially since we’ll replace the Toolbar with the new Navigation (hopefully it’ll land 10.3.x as experimental). @saschaeggi created a lot of good designs in #87 so you could use any of those for the Toolbar since they look fine. I haven’t heard anything from Andrew in a long time and I don’t have any other tool than Slack, so I would suggest to ask other accessibility maintainers like @rainbreaw @mgifford or @bnjmnm and unblock this.

Once again, can the two of you please talk to each other instead of having the rest of us shuttle proposals, concerns and counter-proposals back and forth?

Beyond that, @dww I’m really sorry you’re burned with this but I assumed Sascha’s contributions cleared the path, that’s why I didn’t comment again (in the issue, but we've discussed this several times on Slack in the last months). We’re several Claro and Accessibility maintainers with the same authority, and any of us can decide things, like what is happening on comment #122.

The only reason I'd suggest @camilledavis to not work on this would be because Toolbar is going to be replaced (hopefully soon), not because the people working in the issue can't reach an agreement. So I'll leave to the people working here or yourself @dww to un-postpone this if you want to keep working in this.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

@kostyashupenko good catch! I'm wondering if there's a way to do this all at one and maybe this fix will solve this issue so we don't have to add this extra attribute to every component? https://www.drupal.org/project/navigation/issues/3402592#mr174-note266578 ✨ Reset theme css. Needs work

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thanks @kostyashupenko! Merging this in then :)

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thanks @kostyashupenko! I left a few comments in the MR plus:

This should be removing any default margin the button has to it's adjusted to the top bar and looks minimally OK:

I find it unclear when i'm scrolling down the page and there is no "Save" button in the bottom.

I totally understand this, and the reason is that you're used to find the button there. We discussed this and having the button in 2 places didn't work. We're basically trying to do a change of paradigm here on full page forms.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

I can disable the Toolbar, but then with Umami Editor and Authors don't have a way to access the administration menus anymore. Not sure if this should be addressed here or in a follow-up.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Moving to NW per comments on the MR.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Moved an issue to post MVP.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Update the roadmap and the work done so far.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Updated with a list of the remaining issues.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Updated list of things to test.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thanks @kostyashupenko!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

My comment from #13 still applies: Shortcuts is a net win for 2 active initiatives so my vote is against removing it from core. Both the new Navigation and the Dashboard are planning on using features from the Shortcuts module.

It is true that it would be great to update the code and functionality it provides, but if we remove it from core it'll be complicated to add it again.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Really good proposal!

And thanks @pdjohnson for taking into account the design world! You suggested "User Experience Designer", but that's just a part of the whole area. I would suggest something along the lines: "UX designer, UI Designer".

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Reviewed and merged! Thanks!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

I agree with @finnsky: both .navigation-block and .toolbar-block are the same component, and this MR keeps the 2 CSS files/components with the title in one and the rest of CSS in the other. But I agree with you @kostyashupenko that CSS and "PHP"-generated classes should match. Here's some the background behind the previous discussion regarding naming and the agreement we reached (before most of the back-end work started): #3387374 and #3386927.

Matching Drupal and BEM structure we have is:
- Navigation&co (wrapper, content...)
- Block (as Drupal block) and .toolbar-block<-- Here is where we need to add the label.
- Menu (as Drupal menu) and .toolbar-menu
- Menu links&co

On a back-end perspective we decided to use Navigation to avoid conflicts with the current Toolbar module. I know they don't match for now and they will have to match before opening a MR to core, but I wouldn't make naming changes in a specific MR that is supposed to only change styles. We'll need to properly discuss and plan the naming strategy now that we have both the front-end and back-end more defined, but let's have a discussion to properly bike-shed on that in another issue ;)

So let's keep the existing .toolbar-block for now and re-structure components or naming in a follow-up.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Adding missing credits.

Thanks everybody that made this possible!

PD. If I've missed somebody please comment here so I can give you credits, even if the issue is marked as Fixed.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Adding credits.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

The bug was introduced trying to follow the pattern to moving the variables to its own component without knowing they would be needed by another one (Media Library). And with this changes we're getting back to the point were variables that belong to a component are on base.
The ideal solution would the one were the 2 components are really the same, but that's not doable. The second ideal solution would be to make these variables name generic enough to be used in two different components, but I'm afraid it might lead to some bikeshedding in the issue (naming tends to bring that).
So I'd focus on preventing it happens again apart from fixing the bug. I would recommend to to add a comment on the variables block that explains those variables are needed in 2 places, both Vertical Tabs and Media Library.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Needs rebase :)

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona
πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

ckrina β†’ changed the visibility of the branch 3408500-move-the-save to hidden.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Merged. Thanks all!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

This needs to be manually rebaded.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Answers to #6:

Each form can have several submit buttons

Ideally, we shouldn't have several Submit buttons. The other ones should be regular buttons and/or links.

The solution above is already working, but i don't think it's too much accessible honestly, correct me if i'm wrong.

See the link on the summary to Stackoverflow where explains that the main button with the attribute form should be enough to associate a button to a form-owner in modern web browsers (the ones supported by core).

Other thoughts:

1.I think this task should be not only about moving "Save" button in top bar, but instead we should move all actions of the form in top bar.

Not really: we'll probably more the Delete action somewhere else more hidden to leave more space in the top bar.

2. On which pages we should do this trick? In Drupal there are many pages in admin back office with lots of forms, which has actions. I bet for any admin page where we have form, isn't it ?

This should only happen on Entity forms for now.

3. Potentially one admin page can contain two or more forms (but i can not be sure here) - obviously we shouldn't move all actions from all forms in top bar. What to do in that case?

Exactly! That's why we should move it only for Entity forms now. I forgot to add it in this specific issue. I've updated the form.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

ckrina β†’ changed the visibility of the branch 3412116-add-contextual-link to active.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

ckrina β†’ changed the visibility of the branch 3412116-add-contextual-link to hidden.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Merged, thanks!
And for the JS improvements, feel free to open a new issue as @finnsky says so those specific suggestions can be discussed without blocking this :)

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Merged, thanks all!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

As mentioned in the call today, the contextual link doesn't have to be sticky. The menu itself is more important than accessing the admin page to manage the menus. Adding/rearranging menus might happen 2-3 time per project, using the menu continuously. So the priority is that the menu can be easily used regardless of the contextual link: meaning contextual links shouldn't get in the way.

That said, and since I can't access this Tugboat environment and judging from the screenshots: the icon should land on the margin on top of the first menu link, but not on top of the logo. It should actually wrap the menus, not the whole area. Quick idea of what I'm trying to explain:

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

As mentioned in Slack and the call and to be sure the decision gets reflected and traceable in the future, commenting here. Separating the Help link from the rest of the Administration menu is actually one of the first things we clearly saw on the first design iterations without needing to do research for that. Its priority on an Information Architecture is in another scale from the Administration menu, independently on why historically it ended up in that menu. So its placement shouldn't be linked to the admin menu. Also, because of it's purpose, its place is supposed to be always accessible and reachable, but always in the same place if you are logged in (so you get the Navigation toolbar).

That said, and as discussed on today's meeting, we'll need to find the proper way to detach the existing Help link on the Navigation menu to place it into the footer.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Fixed. Thank you!!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Adjusting summary.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Merged. Thanks all!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Merged! Thanks for the work!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Merged. Great work!! πŸ™ŒπŸΌ

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Yeah I can't think of one right now, but if we find it let's open a new issue for it then. Thanks!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

I opened&closed the MR to see if Tugboat appeared.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Postponing this until designs are more advanced to see the different visual solutions and avoid people investing time here.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Rename β€˜navigation sections’. Consensus amongst those in the meeting was 'Navigation components'. @ckrina to solicit some buy in on this, as the change is tedious to make. It's a machine name that gets used in the code a lot.

We discussed this with @lauriii: he suggested that we better go with "Navigation block" instead of components because he foresees conflicts and misconceptions with the components word the way it is used in Drupal. So let's go with Navigation blocks" and unblock this issue :)

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Should we be styling the active local task in some special way to highlight it?

Yes, but we don't have designs for it yet. I'd say it can be done in a follow-up.

We are hinging access to the contol bar against the access toolbar permission. Should we be using the toolbar module permission? I think we've also said that we need the control bar for mobile anyway, should this even be conditional on a permission at all?

If we are reusing the top bar for the toolbar on mobile, I don't think it's crazy to tie it to the toolbar? I mean, we'd understand this 2 bars as part of the same UI? But I'm not sure what could happen in the future though, like having users with access to local tasks but not to the sidebar navigation.

There is some inconsistency in what we're calling this thing. Sometimes we say "Top Bar" (eg. issue title), sometimes we say "Control Bar" (eg. MR title). The code is split as well. What are we calling it? Would like to make this consistent in the code.

The reason is that we called Control bar the area we used to open/close the sidebar on mobile. Agreed on moving to use only one: I'd say Top bar is fine since it'll be always on the top. But I'd love to hear what @finnsky thinks.

While testing with a limited access user (eg. Author role in demo_umami), I noticed in the navigation side bar that we show icons for links that the user is not allowed to see, but the link and text is hidden. Leading to awkwardness. Is there an issue for this?

Good catch! There is no issue for this yet.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

We do want a quick way to get to this admin screen from the toolbar/navigation bar itself for usability reasons: it'll make it easier to find the place where it's managed, and it'll mean less clicks. So if a user has the permission to manage the toolbar, the contextual link should be available.

What I would suggest is that the contextual region is around of the content area only, and not the footer or the top header. It's the only region we'll let people customize for now.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

After today's discussion on the weekly meeting we mentioned this needed the following changes:

  1. Agreed with #30. The top bar will need to be always visible because on mobile it's needed to place the open/close navigation button.
  2. On the admin theme (Claro), when you’re editing an existing node, the local tasks in entity forms shouldn’t be visible (because it's duplicated).
  3. The font used on the Top Bar should also be the Inter font.

Feel free to coordinate and some people can take the back-end work and some other the front-end, so this way when we merge this it will be a fairly complete feature.

Thanks all for the work!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

I’ve just manually tested it and it’s really cool seeing this working :D

we talked about limiting this to only the content region. It presently works with both regions, just to keep that possibility open. We can restrict that in future.

1. We should restrict this, yes. Either in here or in a follow-up, I don’t mind. The more important reason is the Usability problem it opens because a huge bottom region will the difficult to use in different breakpoints. Plus it’s supposed to be a β€œcomplimentary” and less important region with the main purpose to give the open/close functionality in desktop: this functionality disappears on mobile and the footer stays at the bottom with only the almost no used User menu.

2. Location of the UI: it just go into Configuration not Structure. It’s a UI to change of an administration UI, not the structure of the site being built. I’d recomment movig this to Configuration > User Interface > Navigation sections.

3. Contextual links. 100% agree with #13. But there should be a quick way to get to the UI to manage this. Maybe a link or something like that. But I’d leave that for a follow-up.

Not sure if exposing the 'Category' in the UI is useful? Probably worth it, who knows how this would get implemented, maybe many plugins would proliferate in contrib.

4. Agreed on keeping it. In the future maybe other kind of custom block could be available, like the bar Umami adds to say it’s a testing site.

Probably we need to have button "Reset to default" in navigation layout settings? I guess can be much more blocks in future. Unexperienced users easy can break all site controls.

5. From #13. I’m not sure about this being a need: this pattern doesn’t exist on the Block Layout UI nor in the menus themselves, and showing a button for that you should clearly explain what will change to, what is in the defaults first. Plus it can actually be a disruptive change for whoever click in there. I’d say this lays into the responsibility of whoever has given the permission to a user?

What about trying to solve this potential stressing situation for newbies with proper module documentation in Drupal.org and maybe linking it with something like β€œSee the recommended defaults”?
 Either way I’d move this to a follow-up too.

Should we consider treating the conversion of jQuery to JS as an independent task for the entire module?

6. 
From #18.3. 
It would be great to not introduce any jQuery at all, but happy with it happening in a follow-up.

7. Why is it called β€œNavigation section” and not β€œNavigation block”? I find it confusing since I expect a region as a place to put several blocks if we rely in "Drupal terminology".

8. If you leave the Footer region without menus you loose the capacity to collapse the sidebar because the Collapse button disappears.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

A) Go to items found. link

I'm not sure it's a really standard and easy to understand pattern, but on a design perspective I'd do the following:

1. Should the Go to items found link remain a link or should it restyled as an action link button?
It should be a link, but styled as an action link.

2. Should the filter more orient towards the bulk actions in regards of styling?
I'd say yes, it should stay in the same horizontal space if there's room.

Test with it applied (the small variation of an Action link):

B) Show filtered-button
For this I don't have a super strong opinion. I've done this 3 tests:

  1. With an Action link:
  2. With the Checkbox:
  3. With a Toggle:

On a visual perspective I'd recommend the first one (with the Action link) because it sets a visual hierarchy with the button next to it, but I don't want to make life harder to some users if there is no way to make it better for screen reader. I think the checkbox also works, and the least I like is the checkbox.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Seeing this as RTBC is great news! I understand this can go further but it can happen in follow-ups, so merging this to be sure the follow-ups are easier to address.

Thanks all!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Adding credits. Let us know in the issue if you contributed in the organization but we missed the credits.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

+1

Beyond his great work he's also shown a really constructive attitude on issue interactions, both giving and receiving feedback, on the Navigation β†’ and other related admin UI issues.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

4. Also added animation for rotate buttons arrows. Not sure if we need it actually.

I really like this small details, thank you.

It looks great in Chrome, but unfortunately, this doesn't work with Firefox:

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Merged! Thanks all for the work on this!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

ckrina β†’ changed the visibility of the branch 3374460-menu-animations to hidden.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Yass!! Thank you both for pushing this forward!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

It looks like this needs a local rebase to manually resolve conflict.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thank all, fixed!

Just a small suggestion if I may ask: it would make my life way easier that when a thread is resolved it is marked as so on a MR, so then I'm sure it's done (since it's not as easy for me to review back-end code) :)

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Committed 7c4decb and pushed to 11.x. Thanks!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

We haven’t decided the direction to take here yet, so this doesn’t expect any work yet.

Open to ideas or suggestions on how to solve this though.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

It is OK to not place this menu on the left sidebar yet. Since the way to place the menu will depend on how πŸ“Œ Convert navigation sections to blocks and use the menu system Active is solved, this can be focused on creating the menu itself only.

About the Blocks 1st level item, I added it on the requirements in purpose. It wasn't implemented on the prototype but we got the feedback that it was confusing since it shows on the Tabs anyway. So for now it needs to go there until we do A/B testing for the final decision.

I've also noticed menu items can't be accessed and edited, and they should.

Thanks both for the work!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thanks @m4olivei!

One of the things we change over the last weeks is that we don't want to add the option to add terms there because it easily escalate to more than 6-7 items that would be the usable limit. So I would entirely remove the code that adds the links for taxonomies.

Another thing that differs from the proposed list of links on the issue summary is listing all the Media bundles. Ideally, only Image and Document would be listed there. But that could be moved to a follow-up if it helps close this issue easier!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Committed 3df04e7 to 11.x and 1050de8 to 10.2.x. Thanks!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Committed 0e9777c and pushed to 11.x. Thanks!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

ckrina β†’ changed the visibility of the branch 3214332-new-library-action-link to hidden.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Committed 5a7e9fa and pushed to 11.x. Thanks!

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Changed the status to NW per @larowlan's feedback. Please change it back to NR if the changes have been made.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Thanks for the proposed solutions, bit this needs to create a new design pattern instead of reusing existing ones. So the next step here is to come up with a better design.

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Committed d8e3d8c and pushed to 11.x. Thanks!

Production build https://api.contrib.social 0.61.6-2-g546bc20