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: Empty/No Panels dashboard with a new design #65161
Conversation
…to choose a library panel to create
You have successfully added a new CodeQL configuration |
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.
Awesome progress 🥳 , I gave it a try on my machine, and is looking pretty nice, I left some questions and comments.
public/app/features/dashboard/components/AddLibraryPanelWidget/AddLibraryPanelWidget.tsx
Outdated
Show resolved
Hide resolved
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.
So good!! Looks awesome and really helpful 🚀
I let some little comments
fontWeight: 600, | ||
}), | ||
headerBig: css({ | ||
fontSize: '32px', |
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 have ${theme.typography.....} for font size
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.
yes, but it's not useful in those cases imo - There are no 32 and 20 options, and I'd have to use theme.typography.h5
for the smaller body texts for Add row and Add library panel, which semantically doesn't make sense to me
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.
mmm no, me neither. isn't there any other? take into account this approach lets you modify all the app if suddenly 32px is not the desired one
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.
Yes, you're right it will be better to allow the single source of truth to apply here as well. I changed the header sections to h1 and h2, and the body sections to h4 and h5. It's better also because the typography we use uses rem
, so in the long run any general changes will affect the details very well.
@Ijin08 would you take a look at this:
and the conversation here: Is it alright to continue with those dimensions?
The typography options we have are here: https://github.com/grafana/grafana/blob/redesign/empty-dashboard/packages/grafana-data/src/themes/createTypography.ts#L118
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.
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.
Awesome!
docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md
Outdated
Show resolved
Hide resolved
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.
Added a suggestion. Thank you for the contribution. Please ping me for a re-review and approval.
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.
Awesome work @polibb 🥳 , code looks nice, also gave it a quick test on my local, and the page looks awesome.
What is this feature?
This feature allows users to easily know what is possible with an empty dashboard. The basic ingredients are on their screen ready to use.
Why do we need this feature?
Because it makes it easy to feel comfortable with grafana dashboards.
Who is this feature for?
Everyone
Which issue(s) does this PR fix?:
Fixes #63938
Please check that: