-
Notifications
You must be signed in to change notification settings - Fork 32
feat(Modal, Dialog, and Alert)!: add new accessibility props to Modal, Dialog, and Alert's close Button #3177
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
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 384da96
☁️ Nx Cloud last updated this comment at |
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.
Nice!! A couple small questions/nits
packages/styleguide/src/lib/Molecules/Modals/Dialog/Dialog.stories.tsx
Outdated
Show resolved
Hide resolved
chokidar "^3.4.0" | ||
|
||
"@babel/code-frame@^7.0.0", "@babel/code-frame@^7.10.4", "@babel/code-frame@^7.12.13", "@babel/code-frame@^7.16.7", "@babel/code-frame@^7.24.7", "@babel/code-frame@^7.25.9", "@babel/code-frame@^7.26.0", "@babel/code-frame@^7.27.1": | ||
"@babel/code-frame@^7.0.0", "@babel/code-frame@^7.10.4", "@babel/code-frame@^7.12.13", "@babel/code-frame@^7.16.7", "@babel/code-frame@^7.24.7", "@babel/code-frame@^7.27.1": |
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.
why do we have all of these yarn.lock changes?
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.
Not entirely sure — but I do the get same result on your ajr-nested-checkboxes
branch by running
npx yarn-deduplicate
and then yarn
🤔
I wonder if it's like Cass did this after I did my emotion bump b.c. a lotta packages were still duped? 🤷
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.
yeah maybe, lets just double check with them on monday
|
||
<Canvas of={ModalStories.CloseButtonCustomization} /> | ||
|
||
If the close button is hidden, you will need to create a custom way to close the modal. You can then use the `isOpen` prop to create other controls that toggle the Modal on and off. |
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.
should we add this line to all the other stories?
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.
Adding this to Dialog, but wondering if it's necessary for Alert — since the onClose
prop kinda forces the user to implement the close logic and not sure if we'd want to encourage folks to work around that
|
||
<Canvas of={ModalStories.Scrollable} /> | ||
|
||
## Focus management |
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.
should we add this section to all the other stories?
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.
Sounds good :)
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.
Ended up not doing this for Alert, but would like to confirm with Cass about some Alert specific things
e.g. do we need all those closeButtonProps, or is it really just the text and alignment.
export const CustomClose: React.FC = () => { | ||
const [isOpen, setIsOpen] = useState(false); | ||
return ( | ||
<> | ||
<FillButton onClick={() => setIsOpen(true)}>Open Modal</FillButton> | ||
<Modal | ||
{...defaultProps} | ||
closeButtonProps={{ hidden: true }} | ||
isOpen={isOpen} | ||
onRequestClose={() => setIsOpen(false)} | ||
> | ||
Close the Modal! | ||
</Modal> | ||
</> | ||
); | ||
}; |
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 know this is existing but should we have a button or something to close the modal in this example? its for custom close but were not showing a custom close
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.
Added for both Modal and Dialog (tho lmk about Dialog b.c. both the Accept/Confirm buttons do close the Dialog lol)
); | ||
}; | ||
|
||
export const FocusManagement: React.FC = () => { |
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'm not totally clear what this example is doing tbh, the 2 buttons dont seem to do anything different unless i'm missing something
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.
were you using keyboard (tab + space/enter)?
when I was using mouse it doesn't really show anything lol
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.
not sure, ill try again when i rereview!
packages/styleguide/src/lib/Molecules/Modals/Modal/Modal.stories.tsx
Outdated
Show resolved
Hide resolved
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
Overview
Updates to the Modal, Dialog, and Alert components to encapsulate props for the closeButton in a single object.
Currently using props like
hideCloseButton
anddisableCloseButton
— with this update, these props andref
,tip
, andtipAlignment
are stored in acloseButtonProps
prop.PR Checklist
Testing Instructions
Don't make me tap the sign.
PR Links and Envs