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.

Sign up for GitHub

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

Jump to bottom

[select] feat(Select): new prop "matchTargetWidth" #4841

Merged
merged 5 commits into from
Aug 16, 2021
Merged

[select] feat(Select): new prop "matchTargetWidth" #4841

merged 5 commits into from
Aug 16, 2021

Conversation

millasml
Copy link
Contributor

@millasml millasml commented Aug 5, 2021 •
edited
Loading

Fixes #2956

Checklist

  • Update documentation

Changes proposed in this pull request:

Added a prop matchTargetWidth that will make the Select component's popover have the same width as the target. This was done using a modifier passed to PopoverProps

Screenshot

Before:
image

After:
image

@millasml millasml changed the title Select has matchTargetWidth option [select] fix: Select has matchTargetWidth prop Aug 5, 2021
adidahiya
adidahiya suggested changes Aug 12, 2021
View reviewed changes
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.

Neat solution, that's cool that we can do this with popper modifiers. It would be great to add this feature to Popover2 as well, but that can be in a separate PR. Left a few code style comments in places I'd like cleaned up.

packages/select/src/components/select/select.tsx
order: 800,
};

popoverProps.usePortal = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this strictly necessary? if so, we should make a note of it in the matchTargetWidth prop documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, it is necessary. I've updated the prop docs with a note!

Copy link
Contributor

Choose a reason for hiding this comment

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

cool 👍🏽

packages/select/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/select/src/common/classes.ts Outdated Show resolved Hide resolved
packages/select/src/components/select/_select.scss Outdated Show resolved Hide resolved
@adidahiya adidahiya changed the title [select] fix: Select has matchTargetWidth prop [select] feat(Select): new matchTargetWidth prop Aug 12, 2021
adidahiya
adidahiya suggested changes Aug 16, 2021
View reviewed changes
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.

nice, almost there! I just have a few small style nits I'd like to fix before merging

packages/select/src/common/classes.ts Outdated Show resolved Hide resolved
packages/select/src/components/select/select.tsx Outdated Show resolved Hide resolved
packages/select/src/components/select/select.tsx
order: 800,
};

popoverProps.usePortal = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

cool 👍🏽

@adidahiya
Copy link
Contributor

docs preview looks good

image

@adidahiya adidahiya changed the title [select] feat(Select): new matchTargetWidth prop [select] feat(Select): new prop "matchTargetWidth" Aug 16, 2021
@adidahiya adidahiya merged commit ae5c1e1 into palantir:develop Aug 16, 2021
@adidahiya adidahiya mentioned this pull request May 16, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adidahiya adidahiya adidahiya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Making select width equal to width of wrapping parent
2 participants
@millasml @adidahiya