-
Notifications
You must be signed in to change notification settings - Fork 87
refactor!: baseline alignment #10320
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
Conversation
8e954af to
d0f6061
Compare
| --_has-helper: ; | ||
| --_no-error: initial; | ||
| --_has-error: ; | ||
| display: inline-grid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: would this also help to get us common features like "help-icon" next to the label or field? It's a pretty common UX pattern seen where people want to place an (i) icon on the right side of the label or on the right side of the field which people could click or hover to get more informations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it might, as the label isn't restricted to one line and truncated with an ellipsis, so it should be easier to render custom content in the label slot. But I don't know how feasible that is in general, regarding accessibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.. I'm also talking more about a slot next to the label.. ;) I don't think it would be a good fit within the label
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I don't see why that couldn't be a similar thing as the required indicator, sitting inside the part=label element.
packages/custom-field/src/styles/vaadin-custom-field-base-styles.js
Outdated
Show resolved
Hide resolved
packages/custom-field/test/visual/base/screenshots/custom-field/baseline/default.png
Show resolved
Hide resolved
| grid-template-rows: | ||
| var(--_helper-below-field, var(--_has-label, auto 0) 1fr var(--_has-helper, auto) var(--_has-error, auto)) | ||
| var( | ||
| --_helper-above-field, | ||
| var(--_has-label, auto) var(--_has-helper, auto) var(--_has-label, 0) 1fr var(--_has-error, auto) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in the daily, we might try to use grid-template-areas instead, see if that makes the code easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing we discussed is to use margins instead of gap for the space between the parts. Then we wouldn't need to worry about empty grid cells creating unnecessary gaps, and could reduce the "logic" with the internal custom properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to use both. I suppose the code is slightly more readable.
8103e1d to
5bebcad
Compare
|
Rebased the PR after merging #10441 where |
|
Damn, I broke the alignment in Safari and Firefox when fields don't have a label. |
| color: var(--vaadin-input-field-error-color, var(--vaadin-text-color)); | ||
| display: flex; | ||
| gap: var(--vaadin-gap-s); | ||
| gap: var(--vaadin-gap-xs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted this into #10462 along with relevant screenshot updates, to make this PR focused on alignment.
The container element is no longer meaningful.
This reverts commit c66b4b8.
This reverts commit f9a56e6.
681bcca to
3cc48fd
Compare
|



.vaadin-*-containerelements irrelevant withdisplay: contents.This change removes the ability to globally set "helper-above-field" variant on all fields with the
--vaadin-input-field-helper-orderproperty. Might still be possible with the new custom properties, but might require two properties instead of one.Before
After