feat(Accordion): added left-aligned toggle variant

[thatblindgeye]

Closes #5426

For context, I went with a medium spacer since it looked closer to the spacing between the toggle icon and toggle text when the text is truncated:

Opted to place the modifier on the top level pf-v5-c-accordion element since having that modifier on the individual pf-v5-c-accordion__toggle elements, there's the possibility to mix and match left and right aligned accordion toggles within the same accordion which isn't ideal. Having it on the top level element would sort of prevent that as the styling wouldn't be correct for the default accordion toggle positioning (the toggle icon would no longer be positioned at the end of the toggle)

[patternfly-build]

Preview: https://patternfly-pr-5987.surge.sh

A11y report: https://patternfly-pr-5987-a11y.surge.sh

[mcoker]

This is good to merge as is, but I left some comments, a little verbose just to knowledge share some 😅

If we aren't going to mark it beta, I'd be interested in your thoughts on the .pf-m-start class name, since we wouldn't be able to change it after the fact.

[mcoker]

👌

[mcoker]

It's a new thing, but we can now use true and false as booleans instead of the string (which returns true for the hbs param regardless the value (eg param="[true/false/chocolate chip cookie]" all return true) 🥳

Suggested change

	{{#> accordion accordion--IsStartAligned="true"}}
	{{#> accordion accordion--IsStartAligned=true}}

[thatblindgeye]

🤯 such a small thing but it feels like a game changer to me 😆

[mcoker]

Just in case you want to try and update any of these other pre-existing params to use true, one sneaky little thing to look for is to make sure we aren't relying on the string value to do something somewhere - eg, to do something like below. There's no harm in just leaving them the way they are now, either.

# Some .hbs file
<button
  aria-expanded="{{accordion-toggle--IsExpanded}}"
  ...

[mcoker]

I might name this something a little more scoped, like .pf-m-toggle-start?

[mcoker]

I'd create new local component vars for these values and use those vars here. That will allow someone to change the column-gap, for example, by doing this

.pf-v5-c-accordion {
  --#{$accordion}--m-toggle-start__toggle--ColumnGap: 42px; // or whatever their custom column gap is
}

As opposed to what's in this PR, they'd need to do

.pf-v5-c-accordion.pf-m-start {
  --#{$accordion}--m-toggle-start__toggle--ColumnGap: 42px; // or whatever their custom column gap is
}

This keeps all the themable vars scoped to the base component class, so they can make all of their var overrides in .pf-v5-c-accordion and not having to mess with additional selectors.

Suggested change

	--#{$accordion}__toggle--JustifyContent: start;
	--#{$accordion}__toggle--ColumnGap: var(--#{$pf-global}--spacer--md);
	--#{$accordion}__toggle--JustifyContent: var(--#{$accordion}--m-toggle-start__toggle--JustifyContent);
	--#{$accordion}__toggle--ColumnGap: var(--#{$accordion}--m-toggle-start__toggle--ColumnGap);

[mcoker]

Good call on this column gap. Looks like right now the thing that creates space between the text and icon is a max-width on the text. I'm not sure why that's there, and a margin or something wasn't used instead? (Here's the original PR for this change, I didn't see anything that stood out as to why -#1357)

Not sure if it makes any difference, but now that you have the column gap, you can probably set --pf-v5-c-accordion__toggle-text--MaxWidth: 0; in the the start aligned selector. I don't think you need a local component var for that since it isn't something we'd want anyone to change.

You may also leave a comment (or create an issue) to apply this column-gap to the default toggle and remove the max-width.

[thatblindgeye]

#6013

[mcoker]

Not sure if we need it, but you could add isBeta so we can iterate on it for a bit if we need to make any changes or anticipate it being a beta prop in react. I probably wouldn't since it's so simple, but totally your call.

[thatblindgeye]

@mcoker made updates based on above. I didn't set the maxWidth var to 0 within the start modifier selector since that causes the text element to have a width of 0; could update it to 100%, but since it doesn't really have much effect/it'd be replaced I didn't include it here. If we want to for now I can push another update, though.

[mcoker]

@thatblindgeye doh! re: the max-width, I meant to say whatever the default is - none or reconfigure it a bit to use revert. But the way it is now is fine, too, since it doesn't appear to do anything.

🎃

[patternfly-build]

🎉 This PR is included in version 5.2.0-prerelease.4 🎉

The release is available on:

• GitHub release
• npm package (@prerelease dist-tag)

Your semantic-release bot 📦🚀

[patternfly-build]

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

• GitHub release
• npm package (@prerelease-bugfix dist-tag)

Your semantic-release bot 📦🚀

[patternfly-build]

🎉 This PR is included in version 5.3.0 🎉

The release is available on:

• GitHub release
• npm package (@prerelease-bugfix dist-tag)

Your semantic-release bot 📦🚀