Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.

[Connect] Conditionally render Access Request navigation menu #1273

Merged
avatus merged 8 commits into from
Oct 20, 2022
Merged

[Connect] Conditionally render Access Request navigation menu #1273

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d194789
Conditionally render Access Request navigation item.
avatus Oct 17, 2022
043d4e6
Add protogen files
avatus Oct 17, 2022
f61054a
Remove syncClusterInfo from logout
avatus Oct 17, 2022
5bc9fc0
Remove outdated comment
avatus Oct 18, 2022
3a1d27a
Swap Identity and NavigationMenu positions in TopBar
avatus Oct 19, 2022
d73d80d
Update access request navigation menu conditional statement
avatus Oct 19, 2022
3a878c8
protofiles
avatus Oct 19, 2022
641a0ce
Merge branch 'master' into michaelmyers/connect/add_cluster_feature_d…
avatus Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/teleterm/src/ui/TopBar/NavigationMenu/NavigationMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ export function NavigationMenu() {
const [isPopoverOpened, setIsPopoverOpened] = useState(false);
const selectorRef = useRef<HTMLButtonElement>();

if (!activeRootCluster) {
if (

@gzdunek gzdunek Oct 19, 2022

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.

Maybe something like this? It seems easier to understand

const shouldShowMenu = !!activeRootCluster?.features.advancedAccessWorkflows;
if (!shouldShowMenu) {
  return null;
}

@ravicious ravicious Oct 19, 2022

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.

Would it make sense to put the access requests menu to the left of profile selector? Otherwise the profile selector switches position when switching between a cluster that has access requests and one that doesn't.

!activeRootCluster ||
!activeRootCluster.features.advancedAccessWorkflows

@ravicious ravicious Oct 19, 2022 •
edited
Loading

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.

I'd assume that when we get a response from endpoint that doesn't actually populate features, such as ListRootClusters, then features is going to be undefined. But I'm surprised to see it's actually the whole struct with fields simply set to false. Interesting, I didn't know that.

@avatus avatus Oct 19, 2022

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 think this is bad design. I've updated the backend to only return the features on the relevant rpcs (such as GetCluster). Although I think in the future we may want to rename ListRootClusters to something like ListRootClustersInProfile since that more clearly describes what it does

@ravicious ravicious Oct 20, 2022

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.

Although I think in the future we may want to rename ListRootClusters to something like ListRootClustersInProfile since that more clearly describes what it does

Another idea for the future would be to utilize the render view pattern that Alan mentioned. But yeah, we're free to completely change the RPCs around getting root cluster details, there's nothing holding us back. They work the way they work right now because we've never had to do more than just read stuff from the disk.

) {
return <></>;
}

Expand Down