Skip to content
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

feat(Modal): allow accepting custom arias, allow passing ReactNode to ModalHeader's title #2702

Conversation

@YossiSaadi YossiSaadi requested a review from a team as a code owner January 5, 2025 16:11
@YossiSaadi YossiSaadi changed the title feat(ModalHeader): allow passing ReactNode to modal's title 8129253861 feat(Modal): allow accepting custom arias, allow passing ReactNode to ModalHeader's title Jan 5, 2025
@@ -9,7 +9,12 @@ interface WithoutDescription {
interface WithDescription {
/**
* Descriptive text or content below the title.
* When supplied, would also add an aria-describedby attribute to the modal dialog element.
* - If you pass a **string**, this will automatically set an internally generated `aria-describedby` on the parent Modal.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, very detailed explanation

*/
title: string;
title: string | React.ReactNode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ElementContent? Or we don't want it because it's less descriptive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can it be an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good point, let's see if a request arises, and we can add it later.

I believe most of the use cases would be wrapped under a single div, to flex it and position it properly, and in worst case people can just use fragment.

I generally don't like the ElementContent rather than just saying ReactNode, I think it is a bit redundant.

@YossiSaadi YossiSaadi merged commit 8201d7f into master Jan 6, 2025
14 checks passed
@YossiSaadi YossiSaadi deleted the feat/yossi/new-modal-allow-passing-reactnode-to-title-prop-8129253861-allow-passing-aria-labelledby-and-aria-describedby--8129247306 branch January 6, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants