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

Allow check fields and selectable fields to render as required #487 #581

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adamkudrna
Copy link
Member

Users may find themselves in a situation where the input is not required (i.e. making the input checked), but they also don't want to render the field as optional because not choosing an option can be perfectly valid. For this case, there is the renderAsRequired prop.

This affects CheckboxField, Radio, SelectField, and Toggle.

Closes #487.

@adamkudrna adamkudrna self-assigned this Dec 3, 2024
@github-actions github-actions bot added the BC Breaking change label Dec 3, 2024
@adamkudrna adamkudrna requested a review from atmelmicro December 3, 2024 12:48
@adamkudrna adamkudrna force-pushed the bc/487-render-as-required branch from 47bcb52 to 0f3b935 Compare December 3, 2024 12:50
@mbohal
Copy link
Contributor

mbohal commented Dec 3, 2024

I think the feature is good (we need it and we know why), but honestly the docs are very confusing to me.

I think we should talk about it before merging - I will schedule this for our Monday meeting.

@bedrich-schindler
Copy link
Contributor

I think the feature is good (we need it and we know why), but honestly the docs are very confusing to me.

I think we should talk about it before merging - I will schedule this for our Monday meeting.

I am approving this as it is fine for me. But I might be influenced by the reason for this change.

But it is true that one more sentence could be helpful. I am missing the detailed explanation why would user might need this and why it is correct approach.

Users may find themselves in a situation where the input is
not required (i.e. making the input checked), but they also
don't want to render the field as optional because not
choosing an option can be perfectly valid. For this case,
there is the `renderAsRequired` prop.

This affects `CheckboxField`, `Radio`, `SelectField`, and `Toggle`.

Closes #487
@adamkudrna adamkudrna force-pushed the bc/487-render-as-required branch from 0f3b935 to 7a65aab Compare December 6, 2024 19:00
@@ -186,6 +186,76 @@ React.createElement(() => {
});
```

### Required State
Copy link
Member Author

Choose a reason for hiding this comment

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

But it is true that one more sentence could be helpful. I am missing the detailed explanation why would user might need this and why it is correct approach.

I rewrote the docs and reworked the example. What do you guys think now?

obrazek

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggestions:

  • emphasize it's just a visual thing
  • when a checkbox can be true or false, not only true
  • consider moving it aside as an edge case

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implicitly required form fields visual
3 participants