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

Align help text and labels for tick elements #3965

Merged
merged 5 commits into from
Sep 10, 2021

Conversation

carkod
Copy link
Contributor

@carkod carkod commented Sep 2, 2021

Done

Align help text with tick elements

Fixes #3402
Fixes #3966

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 relesase (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.
  • Documentation side navigation should be updated with the relevant labels.

@webteam-app
Copy link

Demo starting at https://vanilla-framework-3965.demos.haus

@carkod carkod force-pushed the checkbox-help-text branch 2 times, most recently from 290b484 to 764074c Compare September 3, 2021 18:07
@carkod carkod changed the title WIP Checkbox help text Checkbox help text Sep 3, 2021
@carkod carkod force-pushed the checkbox-help-text branch 11 times, most recently from f247c8f to 42c4570 Compare September 6, 2021 08:10
@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Sep 6, 2021

Hi @carkod the alignment of the secoind line of text looks good, but there's another problem - compare ot the Baseline grid toggle in this screenshot:
image

The checkbox box sits slightly higher in the actual example - is this issue introduced here or present in master as well?

@bartaz
Copy link
Contributor

bartaz commented Sep 6, 2021

@carkod Did you approve Percy on this PR? There is a lot of unexpected changes in screenshots that shouldn't be approved. Many of checkbox/radio examples are broken because of style changes, such as:

Screenshot 2021-09-06 at 11 37 03

Unfortunately I don't see the option in percy to 'unapprove' them, but you should review all the checkbox related examples and see how they are affected by the change.

@lyubomir-popov
Copy link
Contributor

@carkod you should not approve percy yourself, this is another form of visual review that should be done by someone reviewing the pr.

@lyubomir-popov
Copy link
Contributor

@carkod @bartaz just push one fix and it will regenerate percy, then please do not approve yourself - just consult it for things that got broken

@carkod
Copy link
Contributor Author

carkod commented Sep 6, 2021

Hi @carkod the alignment of the secoind line of text looks good, but there's another problem - compare ot the Baseline grid toggle in this screenshot:
image

The checkbox box sits slightly higher in the actual example - is this issue introduced here or present in master as well?

Sorry, which checkbox is slightly higher?
What's currently in master is here https://vanillaframework.io/docs/examples/patterns/forms/checkbox

@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Sep 6, 2021

@carkod
the box drops a little under the text baseline, so it looks properly centered compared to the text:
image

in this branch, it is aligned to the baseline, making it look a bit raised:
image

When you change from block to inline context, you need to take care of vertical-align to ensure nothing moves.

@carkod carkod requested a review from bartaz September 7, 2021 17:51
@bartaz
Copy link
Contributor

bartaz commented Sep 8, 2021

Sorry, still not working, now the "required" asterisk disapeared, probably because of the padding I suggested... I will try to have a look how to fix this.

Screenshot 2021-09-08 at 09 21 49

carkod and others added 3 commits September 9, 2021 13:39
This example shows an Accordion pattern variation with less indentation to accomodate checkboxes
Copy link
Contributor

@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

Copy link
Contributor Author

@carkod carkod left a comment

Choose a reason for hiding this comment

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

LGTM @lyubomir-popov final design review? Bartek made some changes to the required icon

@lyubomir-popov
Copy link
Contributor

+1'd :)

@carkod carkod merged commit 7058c94 into canonical:master Sep 10, 2021
@carkod carkod deleted the checkbox-help-text branch September 10, 2021 07:43
@bartaz bartaz added Feature 🎁 New feature or request and removed Bug 🐛 labels Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants