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

Add support for additional attribute passing to Dropdown component #1764

Conversation

serickson-curology
Copy link
Contributor

Updating the dropdown component to allow for more flexible use in the consuming library. Basically I am removing the explicit option for aria-label and replacing it with support for optional ...rest attributes. Had to update the docs and snapshot to go along with these changes.

@serickson-curology serickson-curology self-assigned this Oct 11, 2022
@serickson-curology serickson-curology changed the title Stephen/restore dropdown support for additional attributes Add support for additional attribute passing to Dropdown component Oct 11, 2022
@serickson-curology serickson-curology changed the base branch from master to add-logo-to-theme-constant October 11, 2022 19:33
@serickson-curology serickson-curology changed the base branch from add-logo-to-theme-constant to master October 11, 2022 19:33
Copy link
Member

@LilCare LilCare left a comment

Choose a reason for hiding this comment

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

I'm definitely not an expert on component libraries, but it seems to me that we would want it to have a discrete number of possible attributes and values. Just curious: do you know what the best practice is here?

@@ -61,7 +61,7 @@ export const Dropdown = <T extends OptionType>({
options,
textAlign = 'left',
value,
ariaLabel,
...rest
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: Are we fine with any attribute? Or do we want to hold onto a list of accepted values? We could then update to validate ...rest and reject any unknown attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would tend to agree on being restrictive on which attributes we allow, but this is a pattern that currently exists in the deployed version of radiance-ui (In the button component, for instance.) and so I went with this solution for passing through more attributes without ballooning the parameter list.

I think it would be interesting to look into a pattern that allows for a bit more control on what is allowed through, but for an internal convenience library I don't know how much benefit we would actually get from it if it requires much of a rework.

@LilCare LilCare closed this May 26, 2023
@LilCare LilCare deleted the stephen/restore-dropdown-support-for-additional-attributes branch May 26, 2023 19:35
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.

None yet

2 participants