-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 list filters & favouriting & archival [WIP] #4868
Conversation
…shboard-list-filters-favouriting
@attekei for things like |
roger |
updated this 9:15pm Ready for first UI review cycle, please note that:
|
…shboard-list-filters-favouriting
f4a7c1a
to
4df781f
Compare
Looking good. Here are some things I noticed:
|
…shboard-list-filters-favouriting
thanks for putting up with my OCD tendencies and moving the icon. |
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.
Just a couple small class usage things but after those are fixed this looks good.
<div className="flex-align-right cursor-pointer text-grey-5"> | ||
<Link to="/dashboards/archive"> | ||
<Icon name="viewArchive" | ||
className="mr2 text-brand-hover Icon Icon-viewArchive" |
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 the Icon element should get these Icon
classes automatically based on the name so no need to duplicate them here.
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.
Icon
class doesn't currently merge them to the provided className
parameter but I will make it do that :-)
|
||
{!noDashboardsCreated && | ||
<Icon name="add" | ||
className="text-brand-hover Icon Icon-add" |
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.
Same deal here r.e. Icon
classes.
<li className="Grid-cell shrink-below-content-size" style={{maxWidth: "550px"}}> | ||
<Link to={Urls.dashboard(id)} | ||
data-metabase-event={"Navbar;Dashboards;Open Dashboard;" + id} | ||
className={cx( |
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.
Don't actually need to use cx
here since there aren't any conditional classes on this element.
8e87c4a
to
d3ca537
Compare
TODO
Maybe skipped for now because of implementation complexity: