Skip to content

[Select] Add required prop #1598

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

Merged
merged 3 commits into from
Oct 11, 2022
Merged

Conversation

norman-ags
Copy link
Contributor

Description

Addresses: #1592

image

<Select.Portal>
<Select.Content className={contentClass()}>
<Select.Viewport className={viewportClass()}>
<Select.Item className={itemClass()} value="">
Copy link
Contributor Author

@norman-ags norman-ags Aug 4, 2022

Choose a reason for hiding this comment

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

Not sure if it's a bug, but I have to add this item with value="", because when I tried with just defaultValue as undefined, it selects the first item when submitting the form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this was fixed already a while ago, could you try removing and double-checking?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you manage to double-check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, sorry I missed this comment. I will do it later after workhours.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update on this @benoitgrelard. This is still happening
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense because the native select underneath has a value by default even when no default value is given: https://codesandbox.io/s/native-select-required-7kb1g0?file=/src/App.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@benoitgrelard benoitgrelard Oct 11, 2022

Choose a reason for hiding this comment

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

Oh I see, it's potentially an issue that it doesn't match because it prevents required to work.
For example if someone doesn't provide a defaultValue or value and uses placeholder on Value, it looks like the Select is waiting for the user to pick a value and therefore should stop form submitting if required, yet it wouldn't.

Maybe if a placeholder is provided, we automatically insert a "" option in the native select.

Copy link
Contributor

Choose a reason for hiding this comment

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

Follow-up: #1711

@benoitgrelard
Copy link
Contributor

Hey @norman-ags,

Would you be able to push a change, or force-push to re-trigger the CI checks. Not sure why they didn't run.

Thanks 🙏

@benoitgrelard benoitgrelard added the PR: In Review This PR is in the process of being reviewed by the team label Sep 30, 2022
@itsfaqih
Copy link
Contributor

itsfaqih commented Oct 7, 2022

Hello @norman-ags, I'd like to recreate your PR if you don't mind since I need this too

@norman-ags norman-ags force-pushed the select-add-required branch from bb2019f to b9267cd Compare October 8, 2022 01:08
@norman-ags
Copy link
Contributor Author

Hi guys. Sorry was on vacation.

Would you be able to push a change, or force-push to re-trigger the CI checks. Not sure why they didn't run.

@benoitgrelard done.

@benoitgrelard
Copy link
Contributor

@norman-ags Could you re-run the version check and add the top-level project as "decline"? Thanks!

@benoitgrelard
Copy link
Contributor

@norman-ags could you rebase your branch?

@norman-ags
Copy link
Contributor Author

@norman-ags could you rebase your branch?

Done, I'll check the bug issue I noticed last time later. Thanks

@benoitgrelard benoitgrelard merged commit e7a935c into radix-ui:main Oct 11, 2022
benoitgrelard added a commit to radix-ui/website that referenced this pull request Oct 11, 2022
andy-hook pushed a commit to radix-ui/website that referenced this pull request Oct 11, 2022
@benoitgrelard benoitgrelard removed the PR: In Review This PR is in the process of being reviewed by the team label Oct 12, 2022
benoitgrelard added a commit to radix-ui/website that referenced this pull request Oct 14, 2022
benoitgrelard added a commit to radix-ui/website that referenced this pull request Oct 17, 2022
* Initial

* Align release notes with latest changes (#468)

* Update release notes

* Create new toast page

* Document onResume, onPause toast props

* Document latest changes (#470)

* Document latest changes

* Update data/primitives/overview/releases.mdx

Co-authored-by: Andy Hook <[email protected]>

Co-authored-by: Andy Hook <[email protected]>

* Add note regarding toast timer fix (#474)

* Document latest Label changes (#475)

* Document latest Label changes

* Update copy

* Apply suggestions from code review

Co-authored-by: Andy Hook <[email protected]>

Co-authored-by: Andy Hook <[email protected]>

* Document 1693 changes (#478)

radix-ui/primitives#1693

* Latest slider updates (#480)

* Latest slider updates

radix-ui/primitives#1695

* Update data/primitives/overview/releases.mdx

Co-authored-by: Andy Hook <[email protected]>

Co-authored-by: Andy Hook <[email protected]>

* Add missing inverted prop to Slider (#482)

* Document latest changes (#481)

* Add selection improvement release note (#483)

* Document Radio Group `disabled` prop (#484)

* Create radio group file

* Add note and prop docs addition

* Add data-disabled attribute

* Document latest slider changes (#489)

* Document latest slider changes

radix-ui/primitives#1696

* Update data/primitives/components/slider/1.1.0.mdx

Co-authored-by: Andy Hook <[email protected]>

Co-authored-by: Andy Hook <[email protected]>

* [Select] Add `disabled` to API Reference (#488)

* [Select] Create docs file

* [Select] Add note and props addition

* Document latest Select changes (#490)

radix-ui/primitives#1598

* Update release note (#491)

* Latest Slots updates (#492)

* Latest Slots updates

radix-ui/primitives#1713

* Fix typo

* Document latest `DropdownMenu`/`ContextMenu` changes (`CheckboxItem` indeterminate) (#493)

* Changelog

* Duplicate files

* Docs changes

* PR feedback

* Document `NavigationMenu` delay props (#495)

* Add version page

* Documentation changes

* Update release filenames and breaking demo changes (#497)

* Tweak release note, add date

* Rename patch pages

* Rename remaining patched packages

* Bump files / versions

* Fix build

Co-authored-by: Benoît Grélard <[email protected]>
Co-authored-by: Faqih Muntashir <[email protected]>
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.

3 participants