Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UX: Implement new top-level nav on admin plugin pages #27126

Merged
merged 8 commits into from
May 29, 2024

Conversation

martin-brennan
Copy link
Contributor

@martin-brennan martin-brennan commented May 22, 2024

This uses a new nav style with the heirarchy:

Breadcrumbs
  |- Title
    |- Description
      |- Third-Level Navigation

The navigation bar uses the transparent red-underlined
buttons similar to the user activity page.

Over time all admin pages will use this, but this starts
with the new plugin show page.

image

This uses a new nav style with the heirarchy:

```
Breadcrumbs
  |- Title
    |- Description
      |- Third-Level Navigation
```

The navigation bar uses the transparent red-underlined
buttons similar to the user activity page.

Over time all admin pages will use this, but this starts
with the new plugin show page.
@martin-brennan martin-brennan marked this pull request as draft May 22, 2024 05:33
@martin-brennan martin-brennan changed the title WIP: Implement new top-level nav on admin plugin pages UX: Implement new top-level nav on admin plugin pages May 27, 2024
@martin-brennan martin-brennan marked this pull request as ready for review May 27, 2024 00:28
Copy link
Contributor

@Drenmi Drenmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM! 🚀

Is there any concern that this change might break any themes?

<HorizontalOverflowNav
class="nav-pills action-list main-nav nav plugin-nav"
class="plugin-nav admin-plugin-config-page__top-nav"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does including the page name in the class name mean we'll need to duplicate the CSS for each page?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think maybe admin-plugin-config-page will change at some point, we might want a more generic way of representing this, but for now I think it's OK since it's only affecting these plugin pages.

.user-navigation {
--user-navigation__border-width: 4px;
border-bottom: 1px solid var(--primary-low);
@mixin bordered-horizontal-nav($border-width) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I instinctively like this. Seems like its a good step towards more consistent UI. 👍

Comment on lines +77 to +82
--space-1: 0.25rem;
--space-2: calc(0.25rem * 2);
--space-3: calc(0.25rem * 3);
--space-4: calc(0.25rem * 4);
--space-5: calc(0.25rem * 5);
--space-6: calc(0.25rem * 6);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Wonder if we should define this globally?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great! It would allow us to achieve consistent spacing across all our elements. I'll put up a separate PR for that.

@martin-brennan
Copy link
Contributor Author

Is there any concern that this change might break any themes?

@Drenmi I don't think so, themes tend not to touch the admin area (they shouldn't, we don't want them to). @ellaestigoy might have some thoughts on it though and the other comments for CSS that you've left.

@martin-brennan martin-brennan merged commit 2ed8f96 into main May 29, 2024
16 checks passed
@martin-brennan martin-brennan deleted the ux/admin-new-top-level-nav branch May 29, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants