Skip to content

Update TopBar with new navigation items and styles#36368

Merged
avatus merged 17 commits intomasterfrom
avatus/update_topbar
Jan 12, 2024
Merged

Update TopBar with new navigation items and styles#36368
avatus merged 17 commits intomasterfrom
avatus/update_topbar

Conversation

@avatus
Copy link
Copy Markdown
Contributor

@avatus avatus commented Jan 6, 2024

This is part 2 of the PRs to update our TopBar with the new designs. See figma here https://www.figma.com/file/Gpjs9vjhzUKF1GDbeG9JGE/Application-Design-System?type=design&node-id=0-1&mode=design&t=QWzXjivwgbiKAVCp-0

I recommend commit by commit but most are pretty small.

Screenshot 2024-01-05 at 6 51 33 PM

Goes hand in hand with #36310 but they do not rely on each other (both need to be in to complete the task however)

changelog: Moved the resources side bar navigation into the top bar and updated the look and feel

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 6, 2024

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@avatus avatus removed the request for review from ibeckermayer January 6, 2024 00:53
Copy link
Copy Markdown
Member

@ryanclark ryanclark left a comment

Choose a reason for hiding this comment

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

NavigationSwitcher.tsx can be deleted too

Comment on lines 77 to 82
<TeleportLogo />
{feature?.hideNavigation && (
<ButtonIconContainer onClick={handleBack}>
<ArrowLeft size="medium" />
</ButtonIconContainer>
)}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would look strange with the arrow being after the logo - can you move it back?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Spoke with kenny and for now (we will speak more about it tomorrow) we decided to hide the logo/access management button and replace with the arrow. Although, this still leaves the other nav items in the top bar. should they be there when viewing TAG? i can hide everything if necessary

Copy link
Copy Markdown
Member

@ryanclark ryanclark Jan 11, 2024

Choose a reason for hiding this comment

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

Should we change "Access Management" to say "Back to Teleport" when viewing TAG? cc @roraback

Also yes can we hide the other nav items for TAG please?

const Assist = lazy(() => import('teleport/Assist'));

export function TopBar() {
export function TopBar({ navigationItems }: NavigationProps) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can navigationItems come from useFeatures() instead of being passed through to here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I sorta hijacked topmenuitem since it fit and my filter for topmenu items is if it's defined and a Resource category. worked well, and also was able to use the already filtered items based on access

/>
<NavigationButton
selected={feature?.category === NavigationCategory.Management}
to="/web/users"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This URL shouldn't be hard coded

@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from rudream January 11, 2024 08:58
@kimlisa kimlisa self-requested a review January 11, 2024 09:08
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa left a comment

Choose a reason for hiding this comment

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

i accidentally approved 😅

all the topbar icon has a tooltip except for notifications

i always seemed to have a horizontal scroll bar 🤔 (15 inch mac on chrome)

image

triple scroll bars (but i wasn't sure if we were aiming for mobile friendly)

image

some triple scrolling on some access management views (full screen width)

image

}
align-items: center;
`}
to="/web"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

define in config as a route?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I guess it was already defined under cfg.routes.root so i'll use that!

return cfg.routes.accessRequest;
},
};
topMenuItem = this.navigationItem;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of this being a copy of navigationItem, can it be an optional boolean?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the type of topMenuItem is used elsewhere as a component that shares a lot of the same props as navigationItem. I could make the type be TopMenuItem | boolean but that might muck it up instead of just copying over like this

@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@avatus avatus requested review from kimlisa and ryanclark January 11, 2024 23:15
@kimlisa
Copy link
Copy Markdown
Contributor

kimlisa commented Jan 12, 2024

almost there

access request and user locks when checking out, the top checkout header gets hidden by the menu and we can't exit it

image

this is probably unrelated to your PR... but i'm not sure when our row became uncentered:

image

also notification dot for notications should be closer to the bell? (create a access list, and make the audit due the next day and you'll se a notice dot)

current:
image

before:
image

Copy link
Copy Markdown
Contributor

@kimlisa kimlisa left a comment

Choose a reason for hiding this comment

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

everything else looks good! approving with the above ^ addressed

@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@avatus
Copy link
Copy Markdown
Contributor Author

avatus commented Jan 12, 2024

this is probably unrelated to your PR... but i'm not sure when our row became uncentered:

this was related to my PR. adding the height on every HoverTooltip broke a lot of then, so I just passed in the 100% height explicitly on the specific elements up top

also fixed the notification bell.

I lowered the z-index to keep the topbar below the drawer component for RequestCheckout which I believe is the original intention

@avatus avatus requested a review from kimlisa January 12, 2024 03:09
@avatus avatus added this pull request to the merge queue Jan 12, 2024
@avatus avatus removed this pull request from the merge queue due to a manual request Jan 12, 2024
@avatus avatus enabled auto-merge January 12, 2024 17:51
@avatus avatus added this pull request to the merge queue Jan 12, 2024
Merged via the queue into master with commit d88bd6c Jan 12, 2024
@avatus avatus deleted the avatus/update_topbar branch January 12, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants