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

[checkbox] Should we introduce the label property? #980

Closed
limonte opened this issue Sep 7, 2017 · 12 comments · Fixed by #2539
Closed

[checkbox] Should we introduce the label property? #980

limonte opened this issue Sep 7, 2017 · 12 comments · Fixed by #2539
Labels
DX Developer experience issue enhancement New feature or request needs discussion No decision yet, discussion needed vaadin-checkbox

Comments

@limonte
Copy link
Contributor

limonte commented Sep 7, 2017

If we decide to introduce the label property we should define what will have a priority and probably throw a warning if both ways to set a label are used.

@jouni
Copy link
Member

jouni commented Sep 7, 2017

Might be a nice convenience API. Light DOM content should take priority over the property/attribute, IMO. A warning is probably in order, yes.

@limonte
Copy link
Contributor Author

limonte commented Sep 14, 2017

Yeah, checkbox should be aligned with other components and have the label property, in 90% developers don't need anything but a plain string for checkbox labels.

image

@brp-oskar
Copy link

A use case for us is that we ask our customer to accept terms and conditions in a form. We need both label and text.
image
If the customer submits the form without clicking the checkbox, we want to set an error message to the field.

@jouni
Copy link
Member

jouni commented Apr 27, 2018

I’m not sure, but this also sounds like a potential use case for vaadin/vaadin-core#150 (a generic “field wrapper” element).

It might even be that <vaadin-form-item> could/should handle that role.

For exapmle:

<vaadin-form-item label="Terms and conditions" required error-message="You need to agree in order to continue">
  <vaadin-checkbox>Yes, I agree</vaadin-checkbox>
</vaadin-form-item>

@limonte
Copy link
Contributor Author

limonte commented Apr 27, 2018

It might even be that vaadin-form-item could/should handle that role.

Forcing users to use a wrapper component might be not a good idea.

Also, since we already have required and error-message for form components like vaadin-text-field, in order to be consistent, it's better not to remove (and therefore produce a breaking change) those properties, but instead add them to vaadin-checkbox.

@jouni
Copy link
Member

jouni commented Apr 27, 2018

I’m just thinking this would make it easier to share the common functionality, for things like radio-group and checkbox-group.

But if you want to implement it separately for each element and have separate grouping wrappers, that’s fine by me. Maybe checkbox and radio-button should have label, required and invalid as part of their API, just for convenience 🤷‍♂️

And I wasn’t suggesting that we should remove any current functionality from existing elements. Though, <vaadin-text-field> might want to use this common “field wrapper” element internally, though, to reduce duplicate implementation and reduce the possibility of inconsistencies.

@jouni
Copy link
Member

jouni commented Apr 27, 2018

@brp-oskar, btw, what would you expect to happen when a user clicks the ”Terms and conditions” label in your case? I’m expecting it to focus the checkbox, but not actually toggle it.

@brp-oskar
Copy link

I think a wrapper could be useful when creating own webcomponents (we combine two date pickers for a date range field, it would be useful to be able to add a label, required and invalid state for the combined field). Also for a phone number field (combined country picker and text field).

How would the flow component be implemented with the wrapper? We need to be able to use the Binder with the wrapper and the actual checkbox. The Binder should bind to both the wrapper (label, required, error message) and the checkbox (value, invalid).

@jouni, I think only a focus is the best. A radio button group with a wrapper should only get focus, no select or click.

@jouni
Copy link
Member

jouni commented May 8, 2018

How would the flow component be implemented with the wrapper? We need to be able to use the Binder with the wrapper and the actual checkbox. The Binder should bind to both the wrapper (label, required, error message) and the checkbox (value, invalid).

I don’t have experience or good knowledge of Binder, but I would assume you can create a Flow component that exposes the contained components and their API so that you can access the value and invalid properties (in addition to the wrapper components properties). @Legioth can probably add more details.

@brp-oskar
Copy link

Oh, my question was just meant to be a hint that the solution, chosen for the client side, should also consider how to implement it with Flow (server side) .

@Legioth
Copy link
Member

Legioth commented May 9, 2018

A value input component (aka HasValue or Field) is most easily integrated with Flow it if supports required and readonly properties on the root element. Alternatively, the component integrator can also map the Java methods to any other DOM level action. Otherwise, those features will be no-ops from a UX point of view, but still enforced on the Java side.

@vaadin-bot vaadin-bot transferred this issue from vaadin/vaadin-checkbox May 19, 2021
@vaadin-bot vaadin-bot added DX tests finding enhancement New feature or request needs discussion No decision yet, discussion needed vaadin-checkbox labels May 19, 2021
@vlukashov vlukashov added DX Developer experience issue and removed DX tests finding labels May 19, 2021
@web-padawan web-padawan changed the title Should we introduce the label property to align with other components? [checkbox] Should we introduce the label property? May 20, 2021
@web-padawan
Copy link
Member

This will be done in #2539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Developer experience issue enhancement New feature or request needs discussion No decision yet, discussion needed vaadin-checkbox
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants