-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat: Add orgnaization tabs and settings tab skeleton #5458
Conversation
AdityaHegde
commented
Aug 16, 2024
- Adds tabs for organization that is hidden if the available tabs <= 1
- Adds skeleton for settings page that can be reused in project settings page.
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.
The project settings page looks great. I can definitely re-use it for the public URLs table.
Unsure about the addition of sub-navigation bar (1 in the screenshot). Ideally, we only have 1 nav bar to avoid confusion. Also asked for clarification on the design https://www.figma.com/design/Qt6EyotCBS3V6O31jVhMQ7?node-id=144-815#915050693
Should be Settings instead of Status
Yes @lovincyrus is correct, the intent was for only one tab bar to show at a time. The org tab bar when you're on an org page, the project tab bar when you're on a project page. |
cde4f74
to
fea0389
Compare
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.
@lovincyrus The outline is not really visible to users. The start of rill logo aligns with the tab background when hovered. You can see that in the 1st image you posted. Side note the rill logo doesnt have a hover state in mocks but it does in UI. We should figure it out separately. |
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.
@@ -54,6 +55,7 @@ | |||
$: onReportPage = !!report; | |||
$: onMetricsExplorerPage = isMetricsExplorerPage($page); | |||
$: onPublicURLPage = isPublicURLPage($page); | |||
$: witinOrgPage = withinOrganization($page); |
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.
typo: witin
-> within
<LeftNav | ||
title="Settings" | ||
{basePage} | ||
baseRoute="/[organization]/-/settings" | ||
{navItems} | ||
> | ||
<slot /> | ||
</LeftNav> |
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.
Rather than the main page content being a child of LeftNav
, I think it'd be more conventional/semantic for the main page content and LeftNav
component to be siblings, like:
<div class="layout-container">
<LeftNav {title} {basePage} {baseRoute} {navItems} />
<slot />
</div>
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.
With this approach, it'd also make sense to pull the "Settings" header out of the LeftNav
component.
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.
Sweet. Agree w/ Cyrus that we should reconsider the alignment between the Rill icon & the tabs, but can leave that to a follow-up PR that also encompasses the Project page. Approving to unblock.
Will be taking the comment in a future PR and fix for project tabs as well.