Updating Top Bar Navigation blockers.
First, thanks for all the work put into Tours!
I agree with the issue statement that if there is no Tour available the link should not there. But on a UX perspective, what makes sense for the user is what we should define as the default: so I would argue that there shouldn't be any setting to give the option, the trigger button should just be hidden by default if there is no Tour available.
Another controversial point about this implementation, even if the issue isn't about this, is the placement of the Tour button trigger itself. The way the Navigation & Top Bar are supposed to work is:
- The left sidebar navigation is for site-wide elements, that will appear on all pages regardless on where the user is. That ensures that the Navigation remains consistent across the UI without elements disappearing based on the page (which would confuse users)
- The Top Bar is meant to hold any "contextual" elements relative to the existing page you are in
So based on this, it doesn't make sense to have a Tour link at the top of the main Navigation, taking up valuable space. But if it will also show&hide based on context, its place is not the left sidebar: its place is the Top bar. I see this was agreed in ✨ Integrate with the new experimental navigation module Fixed without any input from the Navigation maintainers, who could have provided this guidance prior to implementing it.
So my recommendation would be to completely remove it from the Navigation (and avoid having to do magic to hide the Tour as a menu item) and implement the Tour on the Top Bar appearing only when necessary.
smustgrave → credited ckrina → .
smustgrave → credited ckrina → .
pameeela → credited ckrina → .
I just reviewed the code and it looks great but since this is providing an standardized solution and goes beyond the Umami message, then its place needs to be more generic and be moved to the top of the content region (above Dashboard, Content, Shortcuts) so it doesn't interfere with the navigation and behaves more as a normal message.
Thanks @finnsky for the proof of concept! Could you move the work into 📌 Proof of concept for the components that will render the top bar Active so we can focus the discussions in this issue to higher level / less detailed topics and the implementation ones in the other? I added it into the summary so it is taken into account too and can be used as reference.
Added also the next steps for the content that can be placed already:
Adding the issue to define those 3 regions: 📌 Define the 3 areas the Top Bar will provide Active .
@matthieuscarset thank you for the suggestion.
We already evaluated the solution you're proposing internally and with the XB team (together with several other ones) and agreed to go with the one defined in comment #19 .
jurgenhaas → credited ckrina → .
We discussed with @lauriii how to integrate the Contextual editing button into the Top Bar and we came up with this solution that will help unblock the Top Bar and the Navigation from becoming stable: move this Edit button (to show contextual areas) into the more actions dropdown.
The primary action would actually be the button to go to Edit the node if you're on a node, which actually has the same label: "Edit". Which makes it really complicated to understand what is this Edit button.
"Edit" label is problematic
Another problem the UI has is that the same label: "Edit" is used both to show the contextual actions and if you're in a node (the Edit option to go into the node). And it makes it really complicated to understand what is this Edit button and differentiate the 2 of them. So a follow-up would be to change the label for this contextual edit button. If it becomes complicated to find the right copy for this label the discussion should be moved to a follow-up so it doesn't block the Navigation from becoming stable.
Change the controller
Another conversation to have in the future is the UI pattern/controller this should be: it should be a toggle since it's showing and hiding the contextual areas that can be edited (I think for accessibility purposes). But please keep any change to this controller for a followup or we won't get this Stable blocker done.
As a final point, to keep in mind that this (almost not used) feature will progressively disappear with the new functionalities XB will provide.
Changing the image with updated designs and some explanation of the areas.
Removing the "prove the need" because it's proven already that this is necessary to fix 📌 Implement a caching strategy for the menu links Active . Discussing this with @plopesc.
Postponing this until ✨ back the ID and aria-labelled-by with JS Postponed is in. Discussing this with @plopesc.
Removing a fixed already issue.
Moving another one into the should have. This is not a critical action for the navigation itself, only if you want to customize it. It should get done, but not block this becoming stable.
tl;dr: I’d suggest taking a route that can eventually provide sets per role and per user, or at least that makes it easy an evolution towards that.
Summary’s pros and cons are based on the existing solutions, and the existing UIs that we have have a really bad UX for the user on all cases. So I wouldn’t base the technical direction on the UX that each solution provides nowadays, even if one if slightly less bad than the others: I’d base it on research around actual user needs or requirements.
For example, I would love to take into account @pcambra’s suggestion coming from his research about builders wanting to have a different set of options per role. That, and the possibility to have private / per users sets.
Our goal is to eventually provide the best version of the features we can (even if it’s for Drupal CMS v1.5, v2 or beyond), and if a technical direction draws out future improvements I’d advise against those tradeoffs. For example, maybe having a set per role could be archived with menus, but I doubt it could provide per user sets eventually.
an improved shortcut module can find its space, either in Drupal core or in the recently created Starshot initiative, providing that it provides sharable, customizable shortcut sets that can be private and per-role. (…) as the Shortcut module might be a better fit for Starshot than Drupal core.
It's worth noting that the same way that shortcuts can be created sitewide, per user and per role, there could be a per-dashboard plugin.
I’d love to highlight also that @pcambra offered to write end evolve the existing Shortcuts implementation towards the direction we decide (at DrupalCon Barcelona 24’). So even though nowadays it might not be what we want, it could evolve to that (inside or outside core). So that wouldn’t imply the Dashboard team having to take on all that extra work.
The definition of that ideal user flow and UIs that would provide a better UX than what we have would need to be defined and implemented over time.
How much of a priority it is to have sets per role would need to be probably more research, but I would dare to say it is not a requirement for v1. That takes out the pressure to get it ready for January, but it should inform the technical direction beyond the initial set of pros and cons.
jjchinquist → credited ckrina → .
pameeela → credited ckrina → .
gábor hojtsy → credited ckrina → .
pameeela → credited ckrina → .
Added designs to the issue summary. This is ready to be worked on.
Just to confirm that this issue should only take care of the item placed on the Navigation itself. Nothing related to the form should be changed, since it really complicates the implementation and we need this issue fixed to get Navigation into stable. So as long we can trigger the old form from the new Navigation we can move any work to a followup (in Workspaces, and discussed & agreed with the Workspaces maintainers).
So, for this MR it'd be great if the color green used is closer to the one from the designs (the green chosen is darker and forces using white text).
The screenshots look exactly as they should, so RTBCing this.
penyaskito → credited ckrina → .
Removing fixed issues.
“there is a common pattern in calendar software to show duration options in an overlay” is not a good enough reason to merge this into Drupal CMS, just a good enough reason to test it and evaluate if we want it.
I'd love that, if possible UX or UI improvements are identified, we highlight them first and discuss them, before assuming they are correct and merging them. Because as a consequence you'll start building things on top of that, others reusing them, and then changing things will be a headache.
So I'd recommend not merging this, but first test it to make sure it's an improvement.
First thanks for trying out the new Navigation.
If I print a page in admin area in portrait mode it does not show the navigation in prints. If I change the layout to landscape mode, the prints also shows the navigation bar.
This might be related to the viewport?
The navigation should not shown on prints at all.
Why shouldn't it? Which is the user need, the justification behind it?
We can't make this kind of assumptions without actually reasoning them :)
We discussed this proposal with the UI Suite team a few times, one of them with the Experience Builder team including Lauri and Wim. They also expressed their interest on seeign this going into core as a helpful step for Experience Builder.
Also, Lauri gave his +1 as a Product Manager to this feature (posting this with his permission).
As co-maintainer of the new core Navigation and as a core FEFM I'm really interested on seeing this happening. Great work so far.
I've done one already to a direct marketing contact, not sharing the recording since it was in Spanish. Please let me know when you come together for the review so we call put together all the findings.
I'm all in with using logical properties everywhere. It makes things way easier to do style with modern CSS.
I haven't looked thought all the code, but I've spotted a a small thing that desn't make a lot of sense:
Being removed: padding: 0 var(--off-canvas-padding) var(--off-canvas-padding);
Being added:
padding-block: 0 var(--off-canvas-padding);
padding-inline: var(--off-canvas-padding);
I'd still prioritize the shorten version of a property on top of the logical one separated because the shorten includes the logical properties too. So I wonder if it's a matter of improving the script?
jjchinquist → credited ckrina → .
Or would you prefer me to revert the commits that move the message?
As mentioned in my message, yes: not merging things that don't follow the designs would be how I would address things. If you want to move quickly I wouldn't do UI changes into this issue that is supposed to focus on "Grant author and editor roles permissions to Navigation module".
If so, it will mean we have tests involving the toolbar even though we don't have toolbar installed any more.
Then, maybe the other issue needs to get in before this one?
Good to see progress on this! But FYI we already have designs for the Umami message integration 📌 Integrate Umami message Needs review after putting some thoughts to it and following the design system we're working on, and it would be great that this would go with those. The left sidebar is supposed to hold all the site-wide information shown on the admin UI, so it doesn't make sense to throw it into the header of the site. Where would you show it int the front-end?
I'd recommend keeping the permissions and the new designs into 2 separated issues so one doesn't block the other.
Good to see progress on this! But FYI we already have designs for the Umami message integration after putting some thoughts to it and following the design system we're working on, and it would be great that this would go with those. The left sidebar is supposed to hold all the site-wide information shown on the admin UI, so it doesn't make sense to throw it into the header of the site. Where would you show it int the front-end?
I'd recommend keeping the permissions and the new designs into 2 separated issues so one doesn't block the other.
pameeela → credited ckrina → .
pameeela → credited ckrina → .
With tour, if the contrib tour module can provide integration, that might work for 10.x, because sites on 10.x can also install that. Either way I would leave tour to last - navigation can be stable in 11.x, and then we can figure out what to do about 10.x from there.
By @catch in Slack.
Next steps in this issue is figure out how to integrate the Tour module. Ideally, a designer should be the one taking next steps here to propose how to integrate Tour with Navigation.
This is a visual change, please add before and after screenshots so we can compare and see the results.
@Gábor Hojtsy yes, those other icons are used on the internal Workspaces UI we're still working on. :)
I gorgot to mention that I opened some follow-ups for the rest of the modules to focus this issue only on Workspaces on the Navigation itself, so the rest of the modules have their own issues:
- 📌 Integrate the Tour module into the Navigation Active
- 📌 Integrate Top Bar Navigation with Contextual editing Active
- 📌 Integrate Umami message Needs review
- 📌 Integrate Announcements into Navigation's drawer/submenu Active
Updating with the issues for core modules that need integrations.
I've updated the first round of designs so the implementation of this can start. This will still miss hover states, for example. I've also added the new icon SVG.
I forgot to add the Navigation stable blocker tag.
Not sure if the component needs to be Navigation or Announcements, so leaving into Navigation for now because we need to provide the designs and styles at least.
wow, this is going fast and I haven't been able to comment before. Thanks all!
As mentioned, we're working on a design for this that will have similar styles as Workspaces (as it's a stable blocker to get Navigation into stable). I know the modules are different, but the UI is similar in a lost of ways. We're working on the styles for the "drawer" once opened, but we haven't finished that yet.
I'm sharing the designs here so you can see the direction (not finished designs yet since we still need to change several things, like the buttons), but I'd recommend not merging this until we've been able to finish the designs.
When we finish the designs for Workspaces, it'll be implemented on the Navigation itself and you'll be able to reuse part of the code without implementing it here.
About the screenshots I'm seeing: I'd recommend trimming the text as we're doing for the icons in the Navigation instead of applying a gradient. See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/navig...
jjchinquist → credited ckrina → .
First, thanks all for working on this and finding a place that needs improvement. I agree with you all on this being one of those places.
We have restored the warning icon before the deprecated text on deprecated modules, just like in the Seven theme.
Even if this existed in Seven, it doesn't mean it'll visually work in Claro: it results in a really packed, confusing and un-structured group of elements. The solution proposed here is not the right one on a design/UI perspective. The title/heading is designed to hold the title, not title + parenthesis and its text + icons. We can't add info and elements to places that are not designed for that when there are no designs.
The goal of this issue is to make more obvious that a module is obsolete. +1 to that. The next step here is to find the right UI solution for Claro (applies to any UI), and that needs a designer designing that new part of the UI that don't exists (even if that existed in Seven, that solution doesn't work here). When that design is aproved, then let's jump into the implementation. This is the best workflow to be sure we end up with digestible UIs. :)
valthebald → credited ckrina → .
So the navigation module will allow a color change and the environment indicator will have to interact with this new API?
I have no idea about the implementation, but I bet nobody has thought about it yet. @plopesc and @m4olivei are the right people to answer this. (@m4olivei was who brought this up to my attention so I'm sure he can help)
Hi @dagmar, thanks for your help with ideas to integrate the Navigation with other modules.
On our designs explorations for the Navigation we already designed for the Environment indicator, in a way that can co-exist with Workspaces. Here are the designs we agreed on, where the initials of the environment are also used as an indicator. They are really close to your explorations, which is great :) Changing the logo background was discarded because the logo can be replaced and either take the whole space or not work well with the color chosen for one of the environments. Credits for this designs should go to @jponch and @anabarcelona.
Either way, I've added this issue to the Navigation roadmap. Thanks!
Adding "Needs subsystem maintainer review" to be sure we get a +1 from a SDC subsystem maintainer. This is a very important first step to integrate SDC into the admin UI and we better get it right :)
Adding 🌱 [META] Migrate Navigation to SDC Active and removing completed tasks.
ckrina → changed the visibility of the branch 3458246-add-subsystem-maintainers to hidden.
Claro already has designs for the blue color, so you can you them for the implementation if you want: https://www.figma.com/design/OqWgzAluHtsOd5uwm1lubFeH/%F0%9F%92%A7-Drupa...
The blue one is targeted to "Status" messages, where the system is informing of something beyond the warning/error/"succeed" type of message. That's why the icon is an "i": it's giving information. Using a bell for it seems more as a "notification" that you have to read, tied to other UI patterns we don't use here.
On the other side, why do you want a gray message? What is its purpose? Which kind of information will it try to communicate? If it's just a communication, why can't it be blue? As a Claro maintainer, I'd love to see this backed with a need instead of opening the floor to any kind of color for messages.
Also, to approve any UI work it would be good to see the messages working with the while interface, not a small piece of it.
Thanks!
pameeela → credited ckrina → .
We would need confirmation from each person that they agree and understand the role as explained in https://www.drupal.org/contribute/core/maintainers#subsystem → .
Personally, I understand the role and I'm happy to continue helping here.
I just pushed the work that @e0ipso helped me do for the Toolbar button component during DrupalCon Portland. It's just initial work so feel free to pick it and follow the work from there.
ckrina → changed the visibility of the branch 3458215-sdc-button-component to hidden.
ckrina → changed the visibility of the branch 3458215-migrate-toolbar-button to active.
ckrina → changed the visibility of the branch 3458215-migrate-toolbar-button to hidden.
Note: I haven’t added as components the following items:
- Admin Toolbar. I see this more as a region rather than a component
- Top Bar. Same, I see this more as a a region
- Body Scroll lock: I see this more as an utility
I know @finnsky had ideas to plan Phase 2, so leaving this here so he can work on it.
pameeela → credited ckrina → .
pameeela → credited ckrina → .
pameeela → credited ckrina → .
pameeela → credited ckrina → .
pameeela → credited ckrina → .
pameeela → credited ckrina → .
This look great! Thanks for working on this and improving the UX.