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

[select] feat(Select): new prop "fill" #4843

Merged
merged 5 commits into from
Aug 24, 2021

Conversation

millasml
Copy link
Contributor

@millasml millasml commented Aug 5, 2021

Fixes #4721

Checklist

  • Update documentation

Changes proposed in this pull request:

Fill prop that makes the select component fill component width.

Screenshot

image

@millasml millasml changed the title [select] fix: Fill option for select WIP: [select] fix: Fill option for select Aug 5, 2021
@millasml millasml changed the title WIP: [select] fix: Fill option for select [select] fix: Fill option for select Aug 5, 2021
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a few comments I'd like addressed

packages/select/src/components/select/select.tsx Outdated Show resolved Hide resolved
childProps.fill = true;
}

const children = React.Children.map(this.props.children, (child: React.ReactNode) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this approach and would prefer not to clone children if we can avoid it. We can't guarantee that the children will support the fill prop, and I'd rather not send along extraneous HTML attributes which might get rendered out to the DOM (and produce a React warning).

If we stick to just rendering {this.props.children}, then <Select> users will have to add fill to children themselves (or style it some other way), but I think that's ok. We should make a note of that in the fill prop documentation.

@adidahiya
Copy link
Contributor

the docs example doesn't work anymore, it needs to be updated so that the button uses fill={true} when this switch is toggled:

2021-08-16 14 33 53

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

works now

2021-08-23 20 44 24

@adidahiya adidahiya changed the title [select] fix: Fill option for select [select] feat(Select): new prop "fill" Aug 24, 2021
@adidahiya adidahiya merged commit 2db16b0 into palantir:develop Aug 24, 2021
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.

[4.0.0-alpha.0] fill doesn't work on MultiSelect, Suggest, Select
2 participants