fix(accordion): add no-box-shadow variation, refactor expanded border by mcoker · Pull Request #2385 · patternfly/patternfly · GitHub

PatternFly-Next preview: https://patternfly-next-pr-2385.surge.sh

are you making this PR a breaking change? or is this considered a bug?

This var isn't used anywhere in the component, so it's safe to remove.

I can add it back and create a follow up issue to remove it in a breaking change release if you can think of a use case where removing it would break someone's design.

I'm not sure if I can think of a use case of it breaking someones design, but to be safe I think we've been following that method right? @mcoker

I think we did leave it one place before in a similar scenario. I think the use case would be if someone had written their own CSS that used --pf-c-accordion--BorderWidth, removing the var would break what they're doing. I'll leave it and leave a comment to remove it at a breaking change release just to be sure.

Would you call this pf-m-plain since this modifier basically removes any border/shadow/line around the accordion?

We could call it something else, but it's just removing the box-shadow at the moment. I don't know if there is anything else we plan on offering in a variation of the accordion without a box shadow.

@mcarrano what do you think, should we give this variation (no box shadow) a more meaningful name? You had also mentioned that you weren't sure why the accordion had a box shadow to begin with. One thing we could do (in a breaking change release) is remove the box shadow from the accordion entirely, and if you want a box shadow, you place it in a card. Though it's worth noting the default box shadow on the accordion is --pf-global--BoxShadow--md, and the box shadow on the card is --pf-global--BoxShadow--sm.

I don't really have a good answer @mcoker . @mceledonia do you know why we put a box shadow around the accordion? I agree that without the shadow, the accordion could be placed in a card to get this effect.

I believe the accordion had the shadow as a stylistic effect for a particular use case where it existed above other content similar to a popover.

As for using a card to achieve the same effect, the default card shadow is quite different than the one I see on the accordion (the card's shadow is smaller/tighter), so I would say as long as the card is flexible enough to utilize whatever type of shadow is needed, that should work.

It makes sense to offer a version without the shadow for usage within a broader context, Kyle worked on this and is out for a while so I will investigate further as I believe this was in collaboration with the digital design team.

Though it's worth noting the default box shadow on the accordion is --pf-global--BoxShadow--md, and the box shadow on the card is --pf-global--BoxShadow--sm.

Woops, missed that! Yeah we would want to be sure we cover that as I have a feeling the box shadow was added to cover a use case where this existed above other content, but I can't say for sure. Going to investigate that like I said - but I think offering a modifier to remove it makes sense.

As for naming, I expect this non-shadow version to be used in a larger context like a form or some kind of content layout. I expect the shadow version to be used more or less on it's own, possibly as a result of an action to trigger it to appear. Maybe that can give some ideas on what to name it.

Sorry for the rapid response, but I am thinking if you need the larger shadow for this, you might be able to utilize a popover to get the same effect. Card would potentially work as well like you previously mentioned. In that case, we could remove that shadow altogether.

@mcoker I'm seeing this, are you as well?

@christiemolloy yep! Updated.

Also created #2388 to clean up these var names.

perfect

Vars that aren't being used:

--pf-c-accordion--BorderWidth
--pf-c-accordion__toggle-text--hover--FontWeight
--pf-c-accordion__toggle-text--active--FontWeight
--pf-c-accordion__toggle-text--focus--Color

Should the items inside .pf-c-accordion__toggle move to root?

Should this move out of __toggle?

Same here, should it move out of __toggle?

@mattnolting I'm seeing these in use still

marvin:patternfly-next cmichael$ grep -ri 'pf-c-accordion__toggle-text--active--FontWeight\|pf-c-accordion__toggle-text--focus--Color' src/patternfly/
src/patternfly//components/Accordion/accordion.scss:  --pf-c-accordion__toggle-text--active--FontWeight: var(--pf-global--FontWeight--semi-bold);
src/patternfly//components/Accordion/accordion.scss:  --pf-c-accordion__toggle-text--focus--Color: var(--pf-global--link--Color);
src/patternfly//components/Accordion/accordion.scss:      font-weight: var(--pf-c-accordion__toggle-text--active--FontWeight);
src/patternfly//components/Accordion/accordion.scss:      color: var(--pf-c-accordion__toggle-text--focus--Color);

Let me know if you still think they need to be removed. Added the other vars to #2388 so we can remove them in a breaking change release.

Perfect 💯

🎉 This PR is included in version 2.39.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

mcoker requested review from christiemolloy and mattnolting October 22, 2019 17:58

christiemolloy suggested changes Oct 22, 2019

View reviewed changes

mcoker force-pushed the issue-2376-2375 branch from 458978e to 8fe8b5a Compare October 23, 2019 20:06

christiemolloy approved these changes Oct 24, 2019

View reviewed changes

mattnolting reviewed Oct 24, 2019

View reviewed changes

mcoker added 3 commits October 24, 2019 10:47

fix(accordion): add no-box-shadow variation, refactor expanded border

1796024

fix(accordion): add unused var back

c21bd69

fix(accordion): fix expanded content left border

8dba7fb

fix(accordion): un-nest toggle element selectors

e0db8dd

mcoker force-pushed the issue-2376-2375 branch from 8fe8b5a to e0db8dd Compare October 24, 2019 15:53

mattnolting approved these changes Oct 24, 2019

View reviewed changes

mattnolting merged commit 2514e35 into patternfly:master Oct 24, 2019

mcoker mentioned this pull request Oct 24, 2019

Accordion: variation to remove box shadow patternfly/patternfly-react#3195

Closed

redallen added the released label Oct 24, 2019

mcoker deleted the issue-2376-2375 branch December 16, 2019 22:53

@@ @@ -4,7 +4,6 @@ @@
               .pf-c-accordion {
                 // accordion
                 --pf-c-accordion--BackgroundColor: var(--pf-global--BackgroundColor--100);
-                --pf-c-accordion--BorderWidth: var(--pf-global--BorderWidth--sm);

-                  padding: var(--pf-c-accordion__toggle--PaddingTop) var(--pf-c-accordion__toggle--PaddingRight) var(--pf-c-accordion__toggle--PaddingBottom) var(--pf-c-accordion__toggle--PaddingLeft);
-                  border: 0;
-                  border-left: var(--pf-c-accordion__toggle--m-expanded--BorderWidth) solid var(--pf-c-accordion__toggle--BorderLeftColor);
+                &.pf-m-no-box-shadow {

-                    max-width: var(--pf-c-accordion__toggle-text--MaxWidth);
-                  }
+                .pf-c-accordion__toggle-text {

-                  &.pf-m-expanded {
-                    --pf-c-accordion__toggle--BorderLeftColor: var(--pf-c-accordion__toggle--m-expanded--BorderLeftColor);
+                .pf-c-accordion__toggle-icon {

fix(accordion): add no-box-shadow variation, refactor expanded border by mcoker · Pull Request #2385 · patternfly/patternfly · GitHub

Uh oh!

fix(accordion): add no-box-shadow variation, refactor expanded border - #2385

fix(accordion): add no-box-shadow variation, refactor expanded border#2385mattnolting merged 4 commits intopatternfly:masterpatternfly/patternfly:masterfrom mcoker:issue-2376-2375mcoker/patternfly-next:issue-2376-2375Copy head branch name to clipboard

Conversation

mcoker commented Oct 22, 2019

Uh oh!

patternfly-build commented Oct 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christiemolloy commented Oct 23, 2019

Uh oh!

mcoker commented Oct 23, 2019

Uh oh!

mcoker commented Oct 23, 2019

Uh oh!

christiemolloy left a comment

Choose a reason for hiding this comment

Uh oh!

mattnolting left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker commented Oct 24, 2019

Uh oh!

mattnolting left a comment

Choose a reason for hiding this comment

Uh oh!

redallen commented Oct 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fix(accordion): add no-box-shadow variation, refactor expanded border#2385
mattnolting merged 4 commits into
patternfly:masterfrom
mcoker:issue-2376-2375