feat: d2l-collapsible-panel > support h5 and h6 semantic levels
#3762
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
|
Visual diff tests failed - pull request #3763 has been opened with the updated goldens. |
jameswklassen
force-pushed
the
jwk/collapsible-panel-h5-h6
branch
from
d02ee6d
to
536b8a1
Compare
Co-authored-by: github-actions <[email protected]>
| const headingStyle = (this.headingStyle === defaultHeading && this.headingLevel !== this.headingStyle) ? this.headingLevel : this.headingStyle; | ||
| this.headingStyle = normalizeHeadingStyle(headingStyle); | ||
|
|
||
| const titleClasses = { | ||
| 'd2l-collapsible-panel-title': true, | ||
| 'd2l-skeletize': true, | ||
| [`d2l-heading-${headingStyle}`]: true, | ||
| [`d2l-heading-${this.headingStyle}`]: true, |
There was a problem hiding this comment.
I noticed that heading-style was appearing in the DOM as the default "3", regardless of what the component did, since we never update this.headingStyle.
Figured it made sense to update the property here...shouldn't change any functionality.
| let headingStyle = (this.headingStyle === defaultHeading && this.headingLevel !== this.headingStyle) ? this.headingLevel : this.headingStyle; | ||
| headingStyle = normalizeHeadingNumber(headingStyle); | ||
| const headingStyle = (this.headingStyle === defaultHeading && this.headingLevel !== this.headingStyle) ? this.headingLevel : this.headingStyle; | ||
| this.headingStyle = normalizeHeadingStyle(headingStyle); |
There was a problem hiding this comment.
I don't love mutating the property here, since if the normalization did something it's going to cause a re-render (and we're already in render()).
There was a problem hiding this comment.
Yeah that's a good point...I forgot why we did it this way.
That said, I'm not crazy about having an incorrect heading-style attribute on the DOM element...maybe the normalization would make more sense in updated? Then we just use this.headingStyle and this.headingLevel directly in this render method.
There was a problem hiding this comment.
I'm not too worried about it, but if you want to ensure that it's always valid I'd suggest creating a getter/setter and doing the validation in the setter. Here's an example from color input.
There was a problem hiding this comment.
The reason the get/set method is better than updated is because updated still happens after render, so 1) the value will be invalid inside render() and 2) "fixing" it will trigger a second render.
There was a problem hiding this comment.
Completely agree.
Also, for cases where we need to compute an internal reactive property in advance of render(), we have willUpdate(...). It's rarely used in core, partly because a lot of the components were written before it existed. There's probably a few cases where we could avoid double rendering by using it. Anyway, just something to keep in mind.
|
Visual diff tests failed - pull request #3767 has been opened with the updated goldens. |
jameswklassen
force-pushed
the
jwk/collapsible-panel-h5-h6
branch
from
56c92b1
to
a6895a7
Compare
|
Visual diff tests failed - pull request #3768 has been opened with the updated goldens. |
Co-authored-by: github-actions <[email protected]>
|
🎉 This PR is included in version 2.131.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
Description
There are use cases that require semantic
h5andh6elements for the panel title. There are no heading styles beyond h4, but the semantic elements can still be supported.If a
h5orh6is selected, it will default to use theh4styles. Consumers can still override styles, like before.Closes #3761