-
Notifications
You must be signed in to change notification settings - Fork 349
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
Add description list basic demo #10184
base: main
Are you sure you want to change the base?
Conversation
Preview: https://patternfly-react-pr-10184.surge.sh A11y report: https://patternfly-react-pr-10184-a11y.surge.sh |
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.
It looks like the content here is pretty different from the content of the core demo, is that something we're concerned with on these @tlabaj ?
packages/react-core/src/demos/DescriptionList/DescriptionList.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/DescriptionList.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/examples/DescriptionListBasic.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/examples/DescriptionListBasic.tsx
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.
Couple more nits below but otherwise looking good to me
packages/react-core/src/demos/DescriptionList/examples/DescriptionListBasic.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/DescriptionList/examples/DescriptionListBasic.tsx
Outdated
Show resolved
Hide resolved
I agree with Austin, the content should match core as closely as possible. Could we update the data in the card to more closely align wit core please. |
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.
Still seeing the divider mismatch on the latest surge. |
@kmcfaul @wise-king-sullyman On local changes look fine but seems changes are not reflecting on surge. |
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.
Yep I see that same thing, not sure why
Seems changes are not reflecting on surge. Please check it on local. It seems fine on local. |
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.
Code looks great! We need to get the previews working to confirm.
2834643
to
dc49a75
Compare
@mattnolting I think the preview is fixed if you wouldn't mind a quick CSS review |
What: Closes #7000
Add description list basic demo
Additional issues: