DRAFT - Theme v2 Insider layout test #5147
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the "Theme v2 insider" feature by adding a new Insider page and its corresponding submodules including story components and a hero section to enhance the layouts. Key changes include:
- Addition of the Insider export and new routing for the Insider page in the layouts index.
- Creation of multiple new components for Insider stories (e.g., StoryCard, various story components, and the Hero component).
- Update of the development script in package.json to build design tokens before starting the dev server.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sandbox/grommet-app/src/pages/layouts/kinds/index.ts | Added export for the new Insider module. |
| sandbox/grommet-app/src/pages/layouts/kinds/Insider/** | New files and folders created to house the Insider page, stories, and hero components. |
| sandbox/grommet-app/src/pages/layouts/index.tsx | Updated routes and imports to include the Insider page. |
| sandbox/grommet-app/package.json | Updated dev script to build design tokens prior to starting Vite. |
| const { colors } = global; | ||
| const { background } = colors; | ||
|
|
||
| console.log('v2 theme - theme', theme) |
There was a problem hiding this comment.
Consider removing or replacing the debug console.log with an appropriate logging mechanism before production deployment.
✅ Deploy Preview for hpe-theme-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for unrivaled-bublanina-3a9bae ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| size="xxlarge" | ||
| margin="none" | ||
| > | ||
| It starts with ambition. |
There was a problem hiding this comment.
It seems like we need a heading flavor for the "condensed" display heading. Also, design wants this all caps. Should we align to start the discussion?
| } | ||
| }, | ||
| edgeSize: { | ||
| '4xlarge': '96px', |
There was a problem hiding this comment.
Is this needed for a responsive reason? 3xlarge is already 96px/
| export const HPEDiscover = ({...rest}) => { | ||
| return ( | ||
| <StoryCard | ||
| level={3} |
There was a problem hiding this comment.
Design calls for these headings to render as 48px (3rem) but they're rendering as 32px (2rem)
| return ( | ||
| <StoryCard | ||
| level={3} | ||
| size={undefined} |
There was a problem hiding this comment.
Design calls for this section's headings to render as 24px (1.5rem) but these are rendering as 18px (1.125rem)
| // console.log('v2 theme - global', global) | ||
| // console.log('v2 theme - colors', colors) | ||
|
|
||
| const adjustedTheme = { |
There was a problem hiding this comment.
I was running into an issue with the theme on primary buttons, where it seemed like grommet was trying to be too "smart" about the dark vs light choice of text/icon color.
I did a workaround for the text color via button.extend in my PR, but clearly missed covering the icon case:
Regardless, something I need to just investigate a little further.
There was a problem hiding this comment.
For some reason it only happens for the rest state, but then "hover" it gets fixed.
Deploy Preview
What does this PR do?
Where should the reviewer start?
What testing has been done on this PR?
In addition to the feature you are implementing, have you checked the following:
General UX Checks
Accessibility Checks
Code Quality Checks
How should this be manually tested?
Any background context you want to provide?
What are the relevant issues?
Screenshots (if appropriate)
Should this PR be mentioned in Design System updates?
Is this change backwards compatible or is it a breaking change?