-
Notifications
You must be signed in to change notification settings - Fork 30
Fix NotAuthorized default actions and clean the code #100
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
Conversation
fhlavac
commented
Jan 31, 2024
- fixed NotAuthorized default primary actions variant to primary
- cleaned the CloseButton props
- Updated docs to contain reference to the parent components props
nicolethoen
left a comment
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 questions about the style overrides in the CloseButton. They could be turned into follow up issues if need be.
| import { createUseStyles } from 'react-jss'; | ||
| import clsx from 'clsx'; | ||
|
|
||
| const useStyles = createUseStyles({ |
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.
@jessiehuff does removing the padding on this component keep the click area large enough to meet a11y standards?
And I think that in general, to keep these components compatible with right-to-left languages, we might want to avoid using float:right. I think there is a float:end or something like that we should be using. I'm also curious though if there's a PF modifier we could use rather than this custom styling. @mcoker WDYT?
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.
@nicolethoen good call on the RTL support. There are logical values for float - "inline-end" (used instead of "right" - what you'd use here) and "inline-start" (used instead of "left").
I'm also curious though if there's a PF modifier we could use rather than this custom styling
We don't have a global modifier for this. We do have float utility classes though, and that utility stylesheet is lightweight (https://unpkg.com/browse/@patternfly/patternfly@latest/utilities/Float/float.css) if that's an option, though it's worth noting that our utility classes are not using logical values when they probably should. I opened an issue for that here patternfly/patternfly#6273.
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.
Based on WCAG guidelines, the clickable area should be 44 x 44 pixels. It looks like removing the padding makes it 48 x 36 so we should probably at least increase the height to 44 pixels.
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.
i think we are going to need a designer to step in at some point to commit a review once or twice a months of new components. with the changes in v6 I can see this giving us headaches. Fortunately we are small enough right now w can fix stuff.
.../patternfly-docs/content/extensions/component-groups/examples/NotAuthorized/NotAuthorized.md
Outdated
Show resolved
Hide resolved
...le/patternfly-docs/content/extensions/component-groups/examples/WarningModal/WarningModal.md
Outdated
Show resolved
Hide resolved
.../patternfly-docs/content/extensions/component-groups/examples/NotAuthorized/NotAuthorized.md
Outdated
Show resolved
Hide resolved
|
A couple of additional phrasing changes (I've already made the changes in the text below): in errorstate.md
in warningmodal.md (I can't preview the interactive example, so I may be wrong about the functionality - lmk if so or if I could better expand)
|
…roups/examples/NotAuthorized/NotAuthorized.md Co-authored-by: Erin Donehoo <[email protected]>
…roups/examples/WarningModal/WarningModal.md Co-authored-by: Erin Donehoo <[email protected]>
…roups/examples/NotAuthorized/NotAuthorized.md Co-authored-by: Erin Donehoo <[email protected]>
|
@nicolethoen @edonehoo thank you, the comments should be addressed. I've also changed type of the |
...le/patternfly-docs/content/extensions/component-groups/examples/WarningModal/WarningModal.md
Show resolved
Hide resolved
|
@edonehoo updated, thank you 🙂 |
|
🎉 This PR is included in version 5.1.0-prerelease.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |