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

Add styling for active submenus on rail nav #1855

Merged

Conversation

AlanGreene
Copy link
Member

@AlanGreene AlanGreene commented Dec 3, 2020

Changes

#803

When the main nav is collapsed to its rail presentation,
ensure we mark any link / menu with an active nav item
as active. This is to help orient users and help guide
them to the relevant section in the expanded nav.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide
for more details.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 3, 2020
to={this.getPath(urls.conditions.all())}
>
Conditions
</SideNavMenuItem>
{this.props.isTriggersInstalled && (
<>
Copy link
Member Author

Choose a reason for hiding this comment

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

Due to the way the active item detection works in the Carbon SideNavMenu, the SideNavMenuItems must be direct children of the SideNavMenu component and cannot be nested in a fragment or other component. This also means we cannot use a custom component that renders the SideNavMenuItem to reduce the duplication.

When the main nav is collapsed to its rail presentation,
ensure we mark any link / menu with an active nav item
as active. This is to help orient users and help guide
them to the relevant section in the expanded nav.
@AlanGreene AlanGreene force-pushed the collapsible_nav_active_submenu branch from 5bf9a03 to d65ecc3 Compare December 3, 2020 12:26
{this.props.isTriggersInstalled && (
<SideNavMenuItem
element={NavLink}
isActive={
Copy link
Member

Choose a reason for hiding this comment

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

Alot of duplication in these isActive parameters, its not big but if you want to move them to a function that takes the url. Ill leave it up to you, how it is is fine too

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I thought about that when I discovered the nesting issue but figured it didn't really bring much benefit so left it. I may have a solution / workaround to the nesting issue for a follow up PR, if that doesn't pan out I'll fix this up then if that's alright.

Copy link
Member

@steveodonovan steveodonovan left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2020
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: steveodonovan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2020
@tekton-robot tekton-robot merged commit 626abc4 into tektoncd:master Dec 4, 2020
@AlanGreene AlanGreene deleted the collapsible_nav_active_submenu branch December 4, 2020 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants