chore(Page): use DashboardWrapper in React demos #7927
Conversation
|
Preview: https://patternfly-react-pr-7927.surge.sh A11y report: https://patternfly-react-pr-7927-a11y.surge.sh |
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
So this is a little confusing, i'll admit - but I think the issue here is actually asking to make sure that the page demos match the Dashboard wrapper - not use the dashboard wrapper. That's because we want consumers to be able to use the example code to build their own page, and they cannot see the code in the dashboard wrapper.
To avoid obfuscating the solution, we need to just update the existing demos so that they match the dashboard wrapper without importing the wrapper. You could even copy/paste a lot of the code from the dashboard wrapper to make it easier.
Yeah I refactored the demos too much probably 😆 should be better now |
tompsota
force-pushed
the
masthead_dashboard_wrapper
branch
2 times, most recently
from
49da3a0 to
ab97662
Compare
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
This looks great!
tompsota
force-pushed
the
masthead_dashboard_wrapper
branch
from
ab97662 to
85e85f1
Compare
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
This looks great. I don't think any of my comments are blockers, they all look like pre-existing issues. Though they be great to fix! And most of the comments apply to all of these demos, and probably the DashboardWrapper, too. I can open new issues if you'd like, just lemme know.
| ); | ||
|
|
||
| const pageNav = ( | ||
| <Nav onSelect={onNavSelect} aria-label="Nav"> |
There was a problem hiding this comment.
| <Nav onSelect={onNavSelect} aria-label="Nav"> | |
| <Nav onSelect={onNavSelect}> |
I think we want this aria-label to be "Global", otherwise the rotor reads this as "Nav navigation". "Global" is the default, so if we want to update, you could remove this entirely.
If so, we should update the main dashboard nav label, too -
This also matches the <PageToggleButton>'s aria-label. Those labels should probably match.
|
|
||
| const sidebar = <PageSidebar nav={pageNav} />; | ||
|
|
||
| const mainContainerId = 'main-content-page-layout-tertiary-nav'; |
There was a problem hiding this comment.
Just a nit, but all of these use this string. Maybe we make it more generic, like 'main-content'?
| Body text should be Overpass Regular at 16px. It should have leading of 24px because <br /> | ||
| of its relative line height of 1.5. |
There was a problem hiding this comment.
Nit and out of scope, but we should update this (and anywhere else it exists) to be generic text. We don't use Overpass anymore, no need for a line break, and the font-size/line-height stuff could change. I can open a new issue if you'd like, just lemme know.
| } | ||
| }} | ||
| > | ||
| <PageSection variant={PageSectionVariants.light}> |
There was a problem hiding this comment.
Probably good to add isWidthLimited here, especially since the card gallery below is limited. Not necessary if the content is short enough, but that's likely what the <br /> in the current content is for - to keep the text from spanning too wide on the screen.
| isTertiaryNavWidthLimited | ||
| isBreadcrumbWidthLimited | ||
| isTertiaryNavGrouped |
There was a problem hiding this comment.
it doesn't look like tertiary nav is defined in any of these demos.
| isTertiaryNavWidthLimited | |
| isBreadcrumbWidthLimited | |
| isTertiaryNavGrouped | |
| isBreadcrumbWidthLimited |
Also having is[...]Grouped without defining the thing that goes in it will create an empty page group section
patternfly-react/packages/react-core/src/components/Page/Page.tsx
Lines 282 to 290 in 38e7a38
@mcoker I updated the demo based on your comments. I also agree that this should be fixed in other demos and in DashboardWrapper. @nicolethoen what do you think about creating a separate issue for that?? |
I agree - go for it! |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
9 participants
What: Closes #7526
I think that this PR could close also #7710 - all demos now use DashboardWrapper and number of cards was increased so it is easier to see the sticky behavior.