Create the Top Bar

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?

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.

Updated IS to reflect the needs of this task.

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

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

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

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.
3. Should we consider implementing a configuration form to enable or disable the Top Bar in the event that we implement permissions?

Thanks

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.

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.

@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!

@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.

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?

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.

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.

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.

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

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!

@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 still needs response from @ckrina.

Keeping it in NW for above.

Thank you.

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!

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

Finished a couple of things:

• Resolved merge conflicts
• 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 beats 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.

Added Inter as font family for top bar.

Before:

After:

Ok @finnsky. Let me check again.

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 metadata 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 conditional 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 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? Out of scope here, just noticed.

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.

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.

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

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

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.

Now that #3411099: Create an administration UI for managing Navigation Blocks is resolved, this MR has a boat load of conflicts.

I'll go through and resolve these.

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.

@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.

Finished reviewing changes. Left comments in the MR.

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.