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

Add a "label-suffix" slot #213

Closed
wants to merge 2 commits into from
Closed

Conversation

heruan
Copy link
Member

@heruan heruan commented Mar 30, 2018

Implementation of #212.


This change is Reviewable

@tomivirkki
Copy link
Member

I would suggest you to write a custom extension for this feature instead. Including this feature in the core text-field element would require a lot of consideration regarding Lumo/Material themes and since it's a new feature and also could be considered a breaking change, this can't be included during the ongoing beta period.

@tomivirkki tomivirkki closed this Apr 4, 2018
@heruan
Copy link
Member Author

heruan commented Apr 4, 2018

Thank you @tomivirkki for the feedback. I understand that merging this in beta period might be unwise due to possible breaking changes, but do you feel this might have a chance to be considered after beta (thus left open)?

Having this as an extension will force to extend also any other elements using vaadin-text-field (such as vaadin-combo-box, vaadin-date-picker, vaadin-dropdown-menu), to enable this feature there.

@tomivirkki
Copy link
Member

A breaking change would in practice require a new major release so unfortunately it's unlikely to make it in <vaadin-text-field> 2.X.

@heruan
Copy link
Member Author

heruan commented Apr 4, 2018

I tried to avoid breaking changes, in fact I don't understand entirely which part of the changes you consider would break existing code using the element. I'm wrapping the label part in a div, but part="label" is still targetable, so existing themes unaware of the wrapper shouldn't break.

@heruan
Copy link
Member Author

heruan commented Apr 9, 2018

@tomivirkki any feedback about what may break merging this PR? I'm willing to try to rework this, if the feature is something you'd consider at least.

@tomivirkki
Copy link
Member

If you open the "Lumo Theme" demo page with the change in place, you can see that the labels are no longer properly aligned in the "Small Size" example:

screenshot 2018-04-09 14 03 01

There might be other such cases that could get easily unnoticed. Since it's in practice a new feature and quite a fundamental change to the internal structure, it would be too risky for us to merge it this close to releasing the stable version.

@heruan
Copy link
Member Author

heruan commented Apr 11, 2018

FWIW, I just fixed the label size issue for vaadin-text-area in the latest commit (not showing up here since the PR is closed now).

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.

2 participants