-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[dashboard] move org menu left #16164
Conversation
started the job as gitpod-build-se-org-menu.1 because the annotations in the pull request description changed |
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-se-org-menu.2 |
b1eb1f5
to
2663b83
Compare
Looking at this now! 👀 |
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-se-org-menu.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes seem like a fast-forward into the future for navigation UX. 🔮
Adding hold from the description, which didn't work.
/hold
<li className="flex-1"></li> | ||
{user?.rolesOrPermissions?.includes("admin") && ( | ||
<li className="cursor-pointer"> | ||
<PillMenuItem name="Admin" link="/admin" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Do know if this was by accident but now that the admin menu is not highlighted when active we could also resolve #15012, as long as we update the header to have a fixed title for the admin. Later on, moving this to the user menu could be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change this in this PR if there are other things that need to be addressed. If not, let's fix it afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened follow-up #16238 to keep track of this. Cc @svenefftinge
@geropl Regarding [4] in #16164 (comment), changing the page title can help, see #16164 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple code related comments - wasn't sure how much of the menu code was just moved vs. new (I think it was mostly just moved?)
Some general feedback:
-
I like the org selector on the left, creates a better understanding of the information architecture hierarchy.
-
It seems odd that we hide the gitpod logo on smaller screens, we should always show the gitpod logo imo.
-
I think it's confusing how we make some of the top menu disappear sometimes, like when creating a new org or project. We should keep showing the same nav up top imo and not hide things.
- Generally I worry with how noisy this makes the top nav. It's a lot, and smaller screens become pretty unusable with how they render. Figuring a better responsive implementation is probably a separate unit of work in itself (I think we have some good designs, we just need to implement them), but I worry this will make that job a bit harder.
<Route exact path="/org-settings" component={TeamSettings} /> | ||
<Route exact path="/org-billing" component={TeamBilling} /> | ||
<Route exact path="/settings" component={TeamSettings} /> | ||
<Route exact path="/billing" component={TeamBilling} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to put all of the org settings pages under a nested route, like /settings
, /settings/billing
, /settings/sso
, etc. It will allow us to simplify some of the way we render those similar pages, and handle shared logic between them, and the grouping feels meaningful as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with that. Could we look at it after this change? I don't believe these links are charged externally already.
} | ||
|
||
export default function Menu() { | ||
const user = useCurrentUser(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I noticed here (and on the team settings sub-menu) - we render a partial nav, then after data completes, we may add additional nav items (like settings and usage). I think it would be a much better experience if we instead loaded the data to determine the full nav menu items, then rendered. This is even more jarring now that they're on the top nav imo. We can utilize react-query to make subsequent renders not need the data calls too. I started something similar for the team settings sub-nav in a PR, and maybe it's a future improvement we can make for the org nav too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Would love to tackle this ina follow up PR.
2663b83
to
3479f91
Compare
I would like to get this merged ASAP, because of the large amount of changes it contains I'm concerned about endless conflict resolution sessions for everything that goes on in parallel. @selfcontained @geropl @gtsiolis I have addressed (and resolved the comment) most of your concerns. What is left could be tackled in a follow up PR. |
/unhold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎨 ✔️
Description
This PR changes the navigation as follows:
/user
slug and removes theorg-
prefix for billing and settings on orgs.TODO: the workspaces list needs to only show workspaces for the selected org, which requires more changes I'll do in a separate PR. Therefore:
/hold
Related Issue(s)
Fixes #16046, #10976
How to test
Check the dashboard in the preview environments for any flaws regarding navigation.
Release Notes
Documentation
Build Options:
Experimental feature to run the build with GitHub Actions (and not in Werft).
leeway-target=components:all
Run Leeway with
--dont-test
Preview Environment Options:
If enabled this will build
install/preview
If enabled this will create the environment on GCE infra
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh