Demo app - Record detail layout #4745
Conversation
|
✅ Deploy Preview for unrivaled-bublanina-3a9bae ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for rad-shortbread-897892 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| } | ||
|
|
||
| const nameProps = { | ||
| width: ['xsmall', 'max-content'] |
There was a problem hiding this comment.
Should we include this in grommet-theme-hpe v6.0.0 as the default?
There was a problem hiding this comment.
oooh I like your thinking.
Perhaps we could do something similar for bottom pad on PageHeader. Maybe audit some other "we always seem to apply these props".
| <Box direction="row" justify="between"> | ||
| <Text>{datum.name}</Text> | ||
| <Box direction="row"> | ||
| <Button icon={<Edit />} size="small" /> |
There was a problem hiding this comment.
The spacing/text hierarchy feels awkward here. Feels like we could use a bit more spacing below "Update firmware"
| key={datum.id} | ||
| background="background-contrast" | ||
| round="small" | ||
| pad={{ left: 'small', right: 'xsmall', vertical: 'small' }} |
There was a problem hiding this comment.
Would "small" all around work?
There was a problem hiding this comment.
Because we have an icon only button on the right, the padding in the button + pad on the container end up making the left-right whitespace feel unbalanced. Reducing the right pad to xsmall feels more balanced.
There was a problem hiding this comment.
I agree locally I changed it to small to be all around the same size but it felt off a little so I think even though the right is not the same visually looks better
| </ContentPane> | ||
| <ContentPane | ||
| gridArea="scheduled-actions" | ||
| heading="Scheduled actions" |
There was a problem hiding this comment.
Getting some overlap here -- not sure if that content pane should have overflow or the page should respond earlier to a single column layout
There was a problem hiding this comment.
Made adjustments to improve responsive behavior.
| if (typeof adjustedStr === 'number') adjustedStr = adjustedStr.toString(); | ||
| adjustedStr = adjustedStr.toLowerCase(); | ||
| return adjustedStr.charAt(0).toUpperCase() + adjustedStr.slice(1); | ||
| } |
| icon: <StatusDisabled />, | ||
| color: 'status-unknown', | ||
| }], | ||
| ]); |
| )} | ||
| </List> | ||
| ); | ||
| } |
|
|
||
| export const DetailSummary: React.FC = () => { | ||
| return ( | ||
| <NameValueList nameProps={nameProps}> |
There was a problem hiding this comment.
locally Im getting a TS error for the nameProps I wanted to check on your side if it was fine or getting same error.
There was a problem hiding this comment.
Yes, I get the error too. I'm leaving it because I think Grommet should update the width type definition for NameValueList as it using Grid underneath and an array with min/max values should be valid. Leaving the error as a reminder that this is something that should be fixed.
There was a problem hiding this comment.
agree I was thinking grommet should be enhanced!
|
|
||
| export const ILOSecurity: React.FC = () => { | ||
| return ( | ||
| <NameValueList nameProps={nameProps}> |
| /> | ||
| } | ||
| skeleton={undefined} | ||
| // animation={["slideLeft", "fadeIn"]} |
|
small comments.. layout and responsive looks good! |
Deploy Preview
What does this PR do?
Where should the reviewer start?
sandbox/grommet-app/src/pages/layouts/kinds/RecordDetailWhat 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?