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

No indentation accordion variation #3963

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

carkod
Copy link
Contributor

@carkod carkod commented Sep 1, 2021

Done

Variation to remove indendation for checkboxes.

Removing 2 rems will adjust the indentation/padding for the default accordion, but if we add checkboxes, it will push the content further, therefore, I removed 1 rem to specifically accomodate for checkboxes.

Therefore, something we need to decides is whether we want to accomodate the indentation for checkboxes or general accordions without checkboxes.

Fixes #3913
Fixes #3885

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.

Screenshots

checkbox-accordion

@webteam-app
Copy link

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

scss/_patterns_accordion.scss Outdated Show resolved Hide resolved
@lyubomir-popov
Copy link
Contributor

@carkod on lod the expanded icon hasa + instead of - icon:
Uploading image.png…

package.json Outdated Show resolved Hide resolved
@carkod carkod force-pushed the accordion-indentation branch 2 times, most recently from 28b5516 to 847fc77 Compare September 2, 2021 09:28
@carkod
Copy link
Contributor Author

carkod commented Sep 2, 2021

@carkod on lod the expanded icon hasa + instead of - icon:

Fixed, thanks!

@carkod
Copy link
Contributor Author

carkod commented Sep 2, 2021

Example of long text/tiny device (220px width):

long-text-preview

@lyubomir-popov
Copy link
Contributor

Example of long text/tiny device (220px width):

this shouldn't happen, any text next to acheckbox should lign to itself. There was a trick with overflow:auto that does this.

@carkod
Copy link
Contributor Author

carkod commented Sep 2, 2021

Example of long text/tiny device (220px width):

this shouldn't happen, any text next to acheckbox should lign to itself. There was a trick with overflow:auto that does this.

mm ok but this is happening in the default checkbox example
https://codepen.io/carkod/pen/eYRzYow

So maybe this should be fixed in checkbox rather than accordion?

@carkod
Copy link
Contributor Author

carkod commented Sep 2, 2021

I approved my percy build, but I suppose someone else needs to check them

@lyubomir-popov
Copy link
Contributor

@carkod why does the spacing in the percy screenshots change between examples?

@bartaz
Copy link
Contributor

bartaz commented Sep 2, 2021

@carkod why does the spacing in the percy screenshots change between examples?

@carkod The style of checkboxes doesn't work in standalone example. Because standalone example uses only accordion. To make checkboxes work we need to add them to standalone SCSS for accordion. We do it for other components that have examples that depend on other styles as well.

I think adding @include vf-p-form-tick-elements; to accordion standalone CSS should make it work.

@lyubomir-popov
Copy link
Contributor

yes, I don't see another valid use atm - maybe bulletted lists - but lets see if sucha need arises and we can add it

@lyubomir-popov
Copy link
Contributor

So maybe this should be fixed in checkbox rather than accordion?

true. can you please file an issue for it?

@carkod
Copy link
Contributor Author

carkod commented Sep 2, 2021

So maybe this should be fixed in checkbox rather than accordion?

true. can you please file an issue for it?

Issue

@carkod carkod force-pushed the accordion-indentation branch 3 times, most recently from 8e7dbe0 to 81ba04a Compare September 2, 2021 14:01
package.json Outdated Show resolved Hide resolved
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.

Standalone example needs adding the checkboxes in it.

So the scss/standalone/patterns_accordion.scss need to @include@include vf-p-form-tick-elements; to make the standalone example contain styling for checkboxes as well

templates/docs/whats-new.md Outdated Show resolved Hide resolved
@carkod carkod force-pushed the accordion-indentation branch 2 times, most recently from b543afe to 82c3367 Compare September 3, 2021 08:47
@bartaz
Copy link
Contributor

bartaz commented Sep 3, 2021

@carkod Thanks for updates. One last thing is adding a label to side navigation to accordion.

So in line:
https://github.com/canonical-web-and-design/vanilla-framework/blob/9e7e04f2f4277c76f89867a4829117e302824a82/templates/_layouts/docs.html#L60

Add a 'updated' as third param, in same way as currtenly forms have it:
https://github.com/canonical-web-and-design/vanilla-framework/blob/9e7e04f2f4277c76f89867a4829117e302824a82/templates/_layouts/docs.html#L51

You can actually remove the one from form, because it's from previous version of Vanilla.

This example shows an Accordion pattern variation with less indentation to accomodate checkboxes
@carkod
Copy link
Contributor Author

carkod commented Sep 3, 2021

@carkod Thanks for updates. One last thing is adding a label to side navigation to accordion.

So in line:
https://github.com/canonical-web-and-design/vanilla-framework/blob/9e7e04f2f4277c76f89867a4829117e302824a82/templates/_layouts/docs.html#L60

Add a 'updated' as third param, in same way as currtenly forms have it:
https://github.com/canonical-web-and-design/vanilla-framework/blob/9e7e04f2f4277c76f89867a4829117e302824a82/templates/_layouts/docs.html#L51

You can actually remove the one from form, because it's from previous version of Vanilla.

Done. Thanks for pointing that out

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, thanks!

@lyubomir-popov
Copy link
Contributor

Looks great thanks @carkod !

@carkod carkod merged commit 4f2e87b into canonical:master Sep 3, 2021
@carkod carkod deleted the accordion-indentation branch September 3, 2021 13:56
@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
Development

Successfully merging this pull request may close these issues.

Accordion: add checkbox example Add ability to reduce left padding to accordion panel
4 participants