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

feat(form): update for optional elements - FRONT-4319 #3543

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

emeryro
Copy link
Contributor

@emeryro emeryro commented Aug 1, 2024

  • allows custom optional text for single checkbox, for consistency
  • fix single checkbox demo controls
  • remove unneeded aria-label in the form demo (no modification of the components themselves)

Copy link

github-actions bot commented Aug 1, 2024

@github-actions github-actions bot temporarily deployed to pull request August 1, 2024 12:14 Inactive
Copy link
Contributor

@planctus planctus left a comment

Choose a reason for hiding this comment

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

I guess this is a side effect of this pr:
image
but it's not entirely clear to me what we are doing here, because also this doesn't seem right:
image
but this would mean informing the form element about the presence of a label from the group template...

@github-actions github-actions bot temporarily deployed to pull request August 8, 2024 09:33 Inactive
@emeryro
Copy link
Contributor Author

emeryro commented Aug 8, 2024

that was partially done, but with some edge case multiple "optional" were displayed. This is fixed.
This idea is to have the default form group status, as in any other form element, and have the one displayed after the checkbox only if it is a single checkbox (it was already the case for the "required" label)

@github-actions github-actions bot temporarily deployed to pull request August 8, 2024 10:39 Inactive
@planctus
Copy link
Contributor

planctus commented Aug 8, 2024

but i don't understand this:
image
guess that when the form group renders a label the marker should only be shown there, even in a single checkbox, no?

@emeryro
Copy link
Contributor Author

emeryro commented Aug 12, 2024

How did you get that display? The last fix I pushed should prevent this...
Indeed, if the checkboxes are in a form group, there should be no "optional" on single checkbox

@planctus
Copy link
Contributor

in the preview :)
Just show the label in the single checkbox and you will get this, both for required and optional ;)

@emeryro
Copy link
Contributor Author

emeryro commented Aug 12, 2024

I should be missing something here... Indeed for the single checkbox we can have the optional or required (that was the case before for the required, I just added the optional), but when it is in the form group, single checkboxes no longer have this extra label
image
image

@planctus
Copy link
Contributor

but also a single checkbox can have a form group label no..? might not be mandatory but it is possible, at least, that is the problematic use case when we have the duplicated label for the required/optional, in production it looks like this, since we were not applying the label to the form element:
image

@emeryro
Copy link
Contributor Author

emeryro commented Aug 13, 2024

ok, finally got it, should be fine now

@github-actions github-actions bot temporarily deployed to pull request August 13, 2024 12:52 Inactive
@planctus planctus merged commit 34cb69c into v4-dev Aug 14, 2024
6 of 7 checks passed
@planctus planctus deleted the FRONT-4319-acc-form-optional branch August 14, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants