-
Notifications
You must be signed in to change notification settings - Fork 78
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: pinned headers #1513
feat: pinned headers #1513
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.
Looks great! Left one line comment, feel free to take it or leave it.
Also wondering stylistically whether there should be a vertical border on the sides of the header cells at the edges—it just looks a little odd to have a background color that meets the white background with no border in between. (E.g. on the left of category
, brand
, and sku
in your example video, same on the rightmost columns as well.)
@@ -0,0 +1,2 @@ | |||
<link rel="preconnect" href="https://rsms.me/" /> | |||
<link rel="stylesheet" href="https://rsms.me/inter/inter.css" /> |
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.
Wondering if we should package this CSS in our repo somehow so that rendering our CSS doesn't require connecting to their server.
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.
Inter is our suggested font for using Malloy, but we don't actually ship it as part of the bundled renderer. It is up to the developer using the renderer to load the font themselves if they want to use it. This is similar to what Google Material 3 does, where Roboto is their default font but it is on the app developer to bring that font in https://github.com/material-components/material-web/blob/main/docs/theming/typography.md
In the code snippet above, that is only executed as part of the Storybook that we use to preview our components, so that won't get shipped as part of Malloy renderer.
In terms of where to recommend loading the font from, it is also available on Google Fonts https://fonts.google.com/specimen/Inter?query=inter. I believe I loaded it directly from source in the Storybook because the latest version had just been released and I wanted to try some of the new number variants.
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 dug a bit deeper and it looks like the Google Fonts version of Inter does not include the variable font including the numeric variants, so I actually think recommending loading from the font source website is best.
pinned-headers.mp4
Changes