Create the Top Bar

Created on 16 November 2023, 7 months ago
Updated 1 February 2024, 5 months ago

Problem/Motivation

From the initial prototype tests and design explorations we've discovered that we need a new top section where we'll place several contextual elements.

Proposed resolution

We want to create a new top bar that will hold actions related to the existing elements on the working area or page being visited (since it will be shown in the front-end too) like the Local Tasks in the entity form or full view mode page. Because of that, if this top area doesn't have any content, it shouldn't appear.
This new top section or region won't be shown in the Block Layout for site builders to place content in. The elements shown in there will be placed only by code, which we should make available and extensible by contributed modules so they can improve and integrate more feature.

Remaining tasks

The implementation of this has started back-porting the Control Bar component from the prototype built in 📌 Create a functional prototype of the new top contextual bar Needs work . For now it only has the region and its styles (scripts or anything else).

(we are here >)

  • The next step here is to define the implementation strategy: should it be a region, a group of blocks or something else completely? Keep in mind that different elements will appear on different pages like local tasks and actions.
  • The local-tasks in entity forms like Content nodes and Taxonomy terms need to be moved into the Top Bar.
  • Add a form under Configuration > User Interface. Add a Checkbox there that, when enabled, the Top bar will be printed. When disabled, the Top Bar won't be printed. The default state should be disabled, so we can keep working on this as an experimental functionality without breaking any admin UI.


Latest design exploration for the top bar. Please ignore the design for the sidebar since it's just a design exploration happening under the Layout redesign efforts.

Feature request
Status

Fixed

Version

1.0

Component

Code

Created by

🇷🇸Serbia finnsky

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Pipeline finished with Skipped
    8 months ago
    #42242
  • Issue created by @finnsky
  • 🇷🇸Serbia finnsky

    Gonna work on it.

  • Status changed to Needs review 7 months ago
  • 🇷🇸Serbia finnsky

    Backported minimal MR for control-bar.

  • Merge request !130Control Bar - #3402046 → (Merged) created by finnsky
  • Status changed to Needs work 7 months ago
  • 🇪🇸Spain ckrina Barcelona

    Thanks @finnsky for giving a first round to this! I like the approach the front-end code is taking, but adding a top bar without anything on it wouldn't make this useful on the module. If we merge something, it should already add some value. I'd recommend to add as a requirement at least to print the breadcrumb and/or the go back link here.

    Also, I see this is adding the permission 'access toolbar', which is basically relying on the old toolbar. This should be created in this module for this module, no?

    I think both part 1 and 2 should happen in this issue, or at least a first approximation to step 2?

  • 🇷🇸Serbia finnsky

    It still need some backender work. Added couple of TODOs. Please take a look

    // TODO: Add special suggestion for only navigation module breadcrumbs
    // To avoid suggestion attachment for page breadcrumbs.
    // Also hide page breadcrumbs for Claro only.

  • 🇷🇸Serbia finnsky

    @ckrina could you please add info about breadcrumbs here? When it will decided.

  • 🇪🇸Spain ckrina Barcelona

    We're having the recap meeting after the tests today. I'll post as soon as I know something else. But regardless the breadcrumb, the top navigation will still be needed. So the back-end planning could still move forward.

  • 🇪🇸Spain ckrina Barcelona
  • 🇪🇸Spain ckrina Barcelona

    Updated IS to reflect the needs of this task.

  • 🇪🇸Spain ckrina Barcelona

    Adding some images to make it easier to understand the idea.

  • 🇪🇸Spain ckrina Barcelona

    Defining the content this issue should actually introduce: not breadcrumbs, yes local tasks in entity forms like Content nodes and Taxonomy terms.

  • 🇪🇸Spain ckrina Barcelona
  • 🇪🇸Spain ckrina Barcelona

    Updating the IS with a more recent design and adding some more details.

  • 🇮🇳India Prashant.c Dharamshala

    Prashant.c made their first commit to this issue’s fork.

  • 🇷🇸Serbia finnsky

    @Prashant.c
    I think you should not apply changes from prototype branch as is.
    It has a lot of quick hardcode solutions.

  • 🇮🇳India Prashant.c Dharamshala

    While working on this issue, I identified additional tasks that require attention. Let's discuss the following points:

    1. The Top Bar is currently visible to all users without any implemented permissions. Should we consider implementing permission settings for access?
    2. Is there a need to relocate the Actions (Save buttons, etc.) to the top? If so, this adjustment would likely involve using form alteration since these elements are integral parts of forms.

    Thanks

  • 🇷🇸Serbia finnsky

    After rebase new css should be checked in terms of font-weights and new variables names

  • 🇪🇸Spain ckrina Barcelona

    Good questions @Prashant.c!

    1. The Top Bar is currently visible to all users without any implemented permissions. Should we consider implementing permission settings for access?

    I would say that it would be better if we can avoid adding permissions if we can handle visibility in a simpler way: if the user doesn't have access to anything printed there (aka the array/whatever value the content goes is empty), then it won't be printed. My concern is that for example we move the Save button there and then a user could not have access to it. But if somebody has a good reason for this I'm missing let's discuss it!

    2. Is there a need to relocate the Actions (Save buttons, etc.) to the top? If so, this adjustment would likely involve using form alteration since these elements are integral parts of forms.

    Yes, but we're still researching what's the best way to properly print the HTML in a way that is accessible. That's why this issue just needs to focus on the local tasks for now.

    3. Should we consider implementing a configuration form to enable or disable the Top Bar in the event that we implement permissions?

    As mentioned in 1, I'd try to keep the configuration at a minimum.

  • 🇪🇸Spain ckrina Barcelona

    I just installed this locally and there are a few things that would need to change:

    1. Breadcrumbs shouldn't be printed on the Top Bar anymore. The work done by @finnsky adding/moving the breadcrumbs should be removed from this issue (sorry @finnsky!)

    2. If the Top bar doesn't have any content to print, it shouldn't be printed.

    The way to control if the Top Bar is printed (when there is content to be printed, as in the node form) should be the form mentioned in the issue summary, instead of permissions.

    @Prashant.c if I've solved all your questions.

  • Status changed to Needs review 6 months ago
  • 🇮🇳India Prashant.c Dharamshala

    @ckrina,

    • I've eliminated breadcrumbs from the Top bar and all associated code/files from the Navigation module. The top bar now exclusively features Local task items for content entities such as content and taxonomy terms.

    • I've introduced a basic settings form accessible at /admin/config/user-interface/navigation/settings for the module, including a "Hide Control bar" checkbox.

    It would be really helpful if someone can review the code.

    Thanks!

  • 🇮🇳India Prashant.c Dharamshala

    @penyaskito
    Appreciate your review. I'd like to address your comments below:

    1. For a theme, it would make sense to use navigation.theme.inc.
    2. To enhance the maintainability of the preprocesser hooks code and prevent the .module file from becoming overly lengthy, I've organized them into separate .inc files. These are then included using the following snippet in the .module file:
          foreach (glob(\Drupal::service('extension.list.module')->getPath('navigation') . '/includes/*.inc') as $file) {
            include $file;
          }
          

    Please let me know if I've addressed your concerns.

    Thank you.

  • Status changed to Needs work 6 months ago
  • 🇨🇦Canada m4olivei Grimsby, ON

    Left some review comments.

    The MR also needs to have some merge conflicts resolved. Also, it would be nice if Tugboat would build a preview here. Anyone aware of why it isnt' by chance?

  • Status changed to Needs review 6 months ago
  • 🇮🇳India Prashant.c Dharamshala

    Tried to fix some points from the MR review comments, I am also not sure why Tugboat preview not showing on the issue even if it has all the required files for it.

  • Assigned to m4olivei
  • Status changed to Needs work 6 months ago
  • 🇨🇦Canada m4olivei Grimsby, ON

    Can you initiate the work on this, and then I can continue?

    Sure @Prashant.c I'll work on it and push a commit. Stay tuned.

  • Issue was unassigned.
  • 🇨🇦Canada m4olivei Grimsby, ON

    OK, I've done the refactoring that I was suggesting in my MR coments. As well as a couple of other things along the way:

    • Removed includes/html.inc, put logic back into navigation.module
    • Refactored the code in hook_preprocess_html and navigation_local_tasks into a new \Drupal\navigation\NavigationRenderer class
    • Since we're only affecting $variables['page_top'] in the preprocess function, I moved us to using the more targetted hook_page_top. I had to adjust the ordering with a hook_module_implements_alter implementation so that we can run after toolbar module and remove it.
    • I noticed that there was a regression in the navigation preprocess code where the #access key was lost. This was causing a broken navigation to show up for anonymous users. I added it back, and also added it to the control_bar render array

    Still work to do here from @larowlan review comments. Will pass that back for @Prashant.c or others.

  • Status changed to Needs review 6 months ago
  • 🇮🇳India Prashant.c Dharamshala

    Added cache contexts to fix the caching issue and did a minor code refactor.
    changing status the status NR.

  • 🇷🇸Serbia finnsky

    I did quick check. We need to show control bar on mobile still
    It has button expand/collapse sidebar https://gyazo.com/3f308319bb1897a105fc6315c4654819

  • 🇷🇸Serbia finnsky

    @ckrina please take a look at #30
    It needs decision. We cannot hide control-bar. It needed on mobile for expand button.

  • Status changed to Needs work 6 months ago
  • 🇨🇦Canada m4olivei Grimsby, ON

    Left a couple of MR comments. Also, noticed that there is some font discrepancies when the control bar is showing on the front-end of the site. I'm assuming the styles should be adjusted to follow the font styles of the --admin-toolbar-* CSS variables so the font styles of the control bar are consistent with the navigation bar?

    Otherwise this is looking pretty good to me. So close!

  • 🇮🇳India Prashant.c Dharamshala

    @m4olivei

    Thanks again for a review. Addressed the review comments.

    1. I agree font family and size etc. needs to be fixed in the Top bar.
    2. Queries in https://www.drupal.org/project/navigation/issues/3402046#comment-15372914 Create the Top Bar Needs review still needs response from @ckrina.

    Keeping it in NW for above.

    Thank you.

  • 🇪🇸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!

  • 🇨🇦Canada m4olivei Grimsby, ON

    I'm going to resolve the merge conflicts on this branch, and take a crack at point 2 from #34.

  • 🇨🇦Canada m4olivei Grimsby, ON

    Finished a couple of things:

    • Resolved merge conclicts
    • Fixed icon classes that changed in a recent 1.x commit
    • Finished point 2 from #30, the local tasks are now hidden when the top bar is displaying them
    • Went through the open threads on the MR. I've marked all that have been addressed with a ✅. Would be nice to clean those up.

    From prior reviews, there is one item that is still outstanding, @larowlan's comment regarding cacheability metadata. I can try to address that tomorrow if no one else beat me to it.

    If would also be nice if someone with access could close and re-open the MR so that we can trigger a Tugboat build.

  • First commit to issue fork.
  • 🇮🇳India AkshayAdhav Pune, India 🇮🇳

    Added Inter as font family for top bar.

    Before:

    After:

  • 🇷🇸Serbia finnsky

    @AkshayAdhav i think you should add that font to .toolbar-button globally, not in context of `control-bar`

  • 🇮🇳India AkshayAdhav Pune, India 🇮🇳

    Ok @finnsky. Let me check again.

  • 🇨🇦Canada m4olivei Grimsby, ON

    More updates, in summary:

    • I've incorporated the abundance of cacheability metadata that comes along with local tasks.
    • In so doing, I've adjusted how access was being checked. Where we were excluding items from getting to the theme system, which would miss their cacheability metadata, we now pass along the #access key verbatium from the local tasks API to the theme layer. This is more in keeping with how rendering is typically done, and avoids us touching the access stuff.
    • I've added some theme hooks to round out the control bar pieces. There is now control_bar, control_bar_local_tasks and control_bar_local_task. The later was required to have a theme hook to wrap both the list item and link, and hang the #access mertadata off of. If access on a particular local task is not allowed then, the theme system will not show the whole list item.

    Along the way I noticed a couple of things:

    • Should we be styling the active local task in some special way to highlight it?
    • 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 contitional on a permission at all?
    • 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.
    • While testing with a limited access user (eg. Author role in demo_umami), I noticed that we show icons for links that the user is not allowed to see, but the link and text is hidden. Leading to awkward. Is there an issue for this? Out of scope here, just noticed.
  • 🇪🇸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.

  • 🇨🇦Canada m4olivei Grimsby, ON

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

    👍

    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?

    OK cool. I agree. If it becomes an issue, easy enough to change down the road.

    Good catch! There is no issue for this yet.

    OK, I'll put it on my list to file a follow up issue here.

  • Status changed to Needs review 5 months ago
  • 🇨🇦Canada m4olivei Grimsby, ON

    I've filed an issue for addressing the issues with limited access users 🐛 Icons display for items that a limited access user does not have access too Active .

    Also sounds like we're in a review state, so updating status.

  • Status changed to Needs work 5 months ago
  • 🇮🇳India AkshayAdhav Pune, India 🇮🇳

    Hi @finnsky
    I wanted to push the code changes but facing some issues while rebasing the branch with 1.x. Will try again tomorrow.

    Regarding the change you requested which needs globally: .toolbar-button { font-family: var(--admin-toolbar-font-family); }, for .toolbar-button class, the font family is already set. Also, the top bar and admin toolbar fall under different divs, so we have to explicitly set the font family to top bar.

  • Assigned to m4olivei
  • 🇨🇦Canada m4olivei Grimsby, ON

    Now that 📌 Create an administration UI for managing Navigation Sections Needs work is resolved, this MR has a boat load of conflicts.

    I'll go through and resolve these.

  • Issue was unassigned.
  • 🇨🇦Canada m4olivei Grimsby, ON

    I've resolved the merge conflicts. I've also done the renaming from "control bar" to "top bar" in places in code that were inconsistent as per #42.

    I'll unassign from me, but leave as Needs work, given ongoing unresolved threads between @finnsky and @AkshayAdhav.

  • 🇷🇸Serbia finnsky

    Gonna work on it

  • 🇷🇸Serbia finnsky

    Corrected the fonts. I leave the task for work because I still need to always display top_bar.

    Perhaps we can use the :has selector here.
    https://developer.mozilla.org/en-US/docs/Web/CSS/:has
    For example

    @media (--admin-toolbar-desktop) {
      .top-bar {
        display: none;
    
        &:has([aria-controls="admin-local-tasks"]) {
          display: block;
        }
      }
    }
    

    Or we may create `top-bar__content` inner. and when it is empty hide it with :has and :empty combination
    https://developer.mozilla.org/en-US/docs/Web/CSS/:empty

  • 🇪🇸Spain plopesc Valladolid

    @m4olivei Took a look into your latest changes and overall looks good.

    Added some comments. Please take a look and share your thoughts about them.

  • 🇨🇦Canada m4olivei Grimsby, ON

    Finished reviewing changes. Left comments in the MR.

  • 🇷🇸Serbia finnsky

    Gonna add dummy button for open tollbar mobile. Just as temporary solution until we not decide what to do with it.

  • Status changed to Needs review 5 months ago
  • 🇨🇦Canada m4olivei Grimsby, ON

    Back-end code looks all good to me! @plopesc, we'll want to file a follow up issue to address the code quality issues around dependency injection.

    From a front-end, integration perspective, the hold-over solution for mobile works as advertised. I'll leave @ckrina to give the final thumbs up, but this is RTBC for me.

  • Status changed to RTBC 5 months ago
  • 🇷🇸Serbia finnsky

    Wow! Great. This is huge step forward! Thanks everyone!

    • finnsky committed 0ed56c17 on 1.x
      Issue #3402046 by Prashant.c, m4olivei, finnsky, plopesc, AkshayAdhav,...
  • Status changed to Fixed 5 months ago
  • Automatically closed - issue fixed for 2 weeks with no activity.

  • Pipeline finished with Success
    4 months ago
    Total: 33s
    #96651
  • Pipeline finished with Success
    4 months ago
    Total: 410s
    #101303
  • Pipeline finished with Success
    4 months ago
    Total: 64s
    #101344
  • Pipeline finished with Success
    4 months ago
    Total: 137s
    #103646
  • Pipeline finished with Failed
    3 months ago
    Total: 266s
    #118247
  • Pipeline finished with Success
    3 months ago
    Total: 185s
    #118343
  • Pipeline finished with Success
    3 months ago
    Total: 221s
    #118379
  • Pipeline finished with Success
    3 months ago
    Total: 230s
    #118404
  • Pipeline finished with Success
    2 months ago
    Total: 1053s
    #140897
  • Pipeline finished with Failed
    about 2 months ago
    Total: 620s
    #151718
  • Pipeline finished with Success
    about 2 months ago
    Total: 347s
    #156372
Production build 0.69.0 2024