-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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(dashboards): Dashboards landing page layout toggle #80790
Conversation
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.
Some small comments but looks good
const FEATURES = [ | ||
'global-views', | ||
'dashboards-basic', | ||
'dashboards-edit', | ||
'discover-query', | ||
]; | ||
|
||
const LAYOUT_KEY = 'dashboards-overview-layout'; |
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.
Maybe we can just export this from the other file?
return dashboardsLayout === 'grid' || dashboardsLayout === 'list' | ||
? dashboardsLayout | ||
: 'grid'; |
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.
We should put 'grid'
and 'list'
in constants at the top of the page to reuse them everywhere instead of dealing with strings
onChange={setDashboardsLayout} | ||
size="md" | ||
value={dashboardsLayout} | ||
aria-label="Layout Control" |
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.
nit, but I think we should wrap aria-labels with t()
because they're user-facing and should also match the user's language
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.
oooo yes good catch
Adds a toggle to change the dashboards layout from grid to table. This is under feature flag and available to only me right now (since there is no functionality to the toggle as of yet). Actual table layout will be implemented in further PRs.
ALSO I am aware that the icon for grid right now is the dashboards icon, Vasudha is working on getting the proper icon for the grid.