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

Fix top margin of validation messages in dense form elements #5343

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Sep 11, 2024

Done

  • Removes negative top margin from form validation error messages & help text
  • Updates .p-form-validation__select-wrapper such that applying .is-dense to the select wrapper also applies dense styling to any input elements inside the select wrapper.

Fixes #5262 , WD-13179

Open question

As implemented, .p-form-validation__select-wrapper.is-dense does not apply dense styling to the input element inside it. However, .p-form-validation__select-wrapper.is-dense is necessary because .is-dense must be at the level of the component that precedes .p-form-validation__message in order to properly remove the negative top margin.

So, we end up with two .is-dense classes:

  • One on .p-form-validation__select-wrapper to remove the negative top margin from the following validation error message
  • One on .p-form-validation__select-wrapper ..p-form-validation__input in order to actually apply the dense styling to the input

This could be resolved by making .p-form-validation__select-wrapper.is-dense automatically apply .is-dense to child elements as well, but I'm not sure if that's desired.

QA

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

Form validation
image

Form help text
image

@webteam-app
Copy link

@jmuzina jmuzina changed the title Fix top margin of validation messages Fix top margin of validation messages in dense form elements Sep 12, 2024
@jmuzina jmuzina added the Question ❓ Further information is requested label Sep 12, 2024
@jmuzina
Copy link
Member Author

jmuzina commented Sep 12, 2024

@bartaz @pastelcyborg @advl Any thoughts on the open question in the PR description?

@bartaz
Copy link
Member

bartaz commented Sep 12, 2024

@bartaz @pastelcyborg @advl Any thoughts on the open question in the PR description?

Form styles are probably one of the oldest in Vanilla, and likely some of component and class naming guidelines were not even fully in place when they were implemented, so it's always a bit of painful and inconsistent to maintain and update those.

This, as far as I understand, only affects select inputs, and only them (for reason I don't know) require this validation wrapper. So not to spend too much time on this, and start refactoring of any sort (we can leave that for new arch) I think it's OK to move is-dense to the wrapper, assuming the old way is still going to work, because you only need this wrapper when validation is in place.

So yes, is-dense applied to the wrapper will affect the nested select input automatically, but you can also use is-dense on select itself.

@jmuzina jmuzina force-pushed the 5262-form-valdation-dense-margin branch from 3a10e5c to c14456d Compare September 12, 2024 16:55
@jmuzina jmuzina removed the Question ❓ Further information is requested label Sep 12, 2024
@jmuzina jmuzina marked this pull request as ready for review September 12, 2024 17:00
@jmuzina jmuzina force-pushed the 5262-form-valdation-dense-margin branch from c14456d to 2c56115 Compare September 12, 2024 17:01
@advl
Copy link
Contributor

advl commented Sep 12, 2024

I agree with @bartaz, would recommend to keep it simple and leave it as it is.

@jmuzina
Copy link
Member Author

jmuzina commented Sep 12, 2024

assuming the old way is still going to work, because you only need this wrapper when validation is in place. So yes, is-dense applied to the wrapper will affect the nested select input automatically, but you can also use is-dense on select itself.

@bartaz

.is-dense is defined as a valid flag class on any element that extends %vf-input-elements .

The following selectors can be used with .is-dense:

// Input styles
[type='text'],
[type='date'],
[type='datetime'],
[type='datetime-local'],
[type='month'],
[type='time'],
[type='week'],
[type='number'],
[type='search'],
[type='password'],
[type='email'],
[type='url'],
[type='tel'],
select {
@extend %vf-input-elements;
}

So, .is-dense on the wrapper does not automatically apply the dense styling to its child select element, as the wrapper is just a <div>. A SCSS change has to be made to allow this behavior.

Screenshots from staging:
.is-dense on just the wrapper (has no effect)
image
.is-dense on the select element inside the wrapper (applies dense styling, but does not affect top margin of the validation message):
image

From this demo:
.is-dense on just the wrapper (applies dense styling to the select element and removes the negative top margin from the following validation error message):
image

@pastelcyborg
Copy link
Contributor

I'm having trouble following this comment thread, but the TLDR for me is that I concur with @bartaz - applying is-dense to any parent element, including the select in this case, should automatically apply to any child elements. That seems to be what's happening in the code in this PR right now, but the comments seem to conflict with this?

@jmuzina
Copy link
Member Author

jmuzina commented Sep 13, 2024

I'm having trouble following this comment thread, but the TLDR for me is that I concur with @bartaz - applying is-dense to any parent element, including the select in this case, should automatically apply to any child elements. That seems to be what's happening in the code in this PR right now, but the comments seem to conflict with this?

@pastelcyborg

Yes, the PR is changing things so .is-dense applied to the parent select wrapper applies the dense styling to the input as well. Just clarifying that a change is needed to support that, since main doesn't support that currently.

Copy link
Contributor

@pastelcyborg pastelcyborg left a comment

Choose a reason for hiding this comment

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

Oh, there's also a minor vertical alignment issue with the icons on the example:

image

@jmuzina jmuzina added the Review: Percy needed This PR needs a review of Percy for visual regressions label Sep 19, 2024
@bartaz bartaz added Review: QA +1 and removed Review: QA needed Review: Percy needed This PR needs a review of Percy for visual regressions labels Oct 21, 2024
bartaz
bartaz previously approved these changes Oct 21, 2024
Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bartaz bartaz added the Feature 🎁 New feature or request label Oct 21, 2024
@bartaz
Copy link
Member

bartaz commented Oct 21, 2024

As we are adding new way of using is-dense with selects and validation, probably worth mentioning it as an update in the changelog and use the Updated label in side nav and docs.

@jmuzina jmuzina force-pushed the 5262-form-valdation-dense-margin branch from 2c56115 to 4afc7fb Compare October 21, 2024 16:10
@jmuzina
Copy link
Member Author

jmuzina commented Oct 21, 2024

As we are adding new way of using is-dense with selects and validation, probably worth mentioning it as an update in the changelog and use the Updated label in side nav and docs.

@bartaz Updated. I tried adding links to the new docs sections in the changelog, but noticed an issue with using hashlinks to docs sections with status labels in their headers. It has been filed as #5393

@jmuzina jmuzina removed the Feature 🎁 New feature or request label Oct 21, 2024
Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jmuzina jmuzina merged commit ade7ef6 into canonical:main Oct 22, 2024
8 checks passed
@jmuzina jmuzina deleted the 5262-form-valdation-dense-margin branch October 22, 2024 12:58
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.

Form validation and help messages don't work with dense elements
5 participants