fix: OPL separators on hidden items and edge cases #3136
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
|
Could not generate new goldens - your code changes will update golden files that you do not have the latest version of. Please rebase or merge |
eKoopmans
force-pushed
the
ekoopmans/opl-separator-improvements
branch
from
fb16010
to
488ba6a
Compare
|
Visual diff tests failed - pull request #3185 has been opened with the updated goldens. |
Co-authored-by: github-actions <github-actions@github.com>
|
Visual diff tests failed - pull request #3187 has been opened with the updated goldens. |
| } | ||
|
|
||
| _setItemSeparatorVisibility() { | ||
| const slottedElements = this._slot.assignedElements(); |
There was a problem hiding this comment.
It looks like we're assuming that the slot element will never change. To avoid the assumption, we could query for the slot when needed, but in this particular case, we can have it passed in...
_onItemsChanged(e) {
this._setItemSeparatorVisibility(e.target); //e.target of slotchange is the slot element
}
_setItemSeparatorVisibility(slot) {
...
}
| firstUpdated() { | ||
| this._slot = this.shadowRoot.querySelector('slot:not([name])'); | ||
| this.addEventListener('d2l-object-property-list-item-visibility-change', this._onItemsChanged); | ||
| if (this._slot.childElementCount) this._onItemsChanged(); |
There was a problem hiding this comment.
Interesting... I never knew the slot had this property. Seems Mozilla isn't forthcoming in these dets.
There was a problem hiding this comment.
Though, are you sure this call is needed in firstUpdated? I think the slotchange event will be dispatched when the nodes are first distributed to it.
There was a problem hiding this comment.
This is specifically for the skeleton scenario, when nothing is slotted. childElementCount is to detect if there is any default content (e.g. skeleton) being rendered. So it's not anything specific to slots, rather just a default element attribute property:
https://developer.mozilla.org/en-US/docs/Web/API/Element/childElementCount
In my testing this was necessary, otherwise the visual-diff for skeleton was rendering a separator on the final item.
I am hopeful that eventually we will be able to do this sort of thing, for example to select an element that has a hidden sibling (or doesn't have a hidden sibling): :host(:has(+ [hidden])) { ... }
:host(:not(:has(+ [hidden]))) { ... }It works in Safari, but it does not work in Chrome. There appears to be some ongoing discussion regarding what types of selectors should be valid in |
Yeah, it does still get messy here with the facts that:
|
|
🎉 This PR is included in version 2.80.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
This PR is a follow-up to #3134. It changes how separators are rendered in object-property-lists to fix an edge case with status indicators, and also adds
hiddensupport! The hidden support should make backporting to Polymer projects a lot easier/more possible in the future.Opening as a draft targeting #3134's branch until it is merged.Rally: https://rally1.rallydev.com/#/?detail=/userstory/674102762601&fdp=true