DE54475 - Fix sticky list controls in dialog issue #3899
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 #3900 has been opened with the updated goldens. |
Co-authored-by: github-actions <[email protected]>
…ill be used in calc
|
If this looks ok, I'm still trying to get this in before branching. But if there's concerns, we can discuss pushing it or certfixing. It definitely felt like everything was against me trying to get this in 😩 |
| @@ -37,6 +38,7 @@ export class ListControls extends SelectionControls { | |||
|
|
|||
| const parent = findComposedAncestor(this.parentNode, node => node && node.tagName === 'D2L-LIST'); | |||
| if (parent) this._extendSeparator = parent.hasAttribute('extend-separators'); | |||
| if (this._extendSeparator) this.style.setProperty('--d2l-selection-controls-padding', '0px'); | |||
There was a problem hiding this comment.
Could this be put in the CSS inside of the .d2l-list-controls-extend-separator block that's already defined?
There was a problem hiding this comment.
That only gets added to the section, but I also need it on the div that acts as the shadow:
But if we like that general pattern better, I could create a new class that gets added to both spots?
There was a problem hiding this comment.
I think that's my preference, to avoid setting CSS variables through JavaScript. But if that gets messy, this is fine.
There was a problem hiding this comment.
Ok, pushed that change. It's not quite as minimal as I'd like, I played around with different ways to get the names shorter but it always exposed a bit too much underlying knowledge of the selection controls
There was a problem hiding this comment.
Chatted with @dbatiste offline about this. We're going to go with the setting the variable in JavaScript for now. The other simple options felt a bit worse (reflecting a state property or passing classes in that exposed too much of how the underlying selection-controls worked). We could go with a larger change to refactor the rendering of the controls, something like this:
render() {
return html`
<div class="extend-separators">this._render()</div>
`;
}
If we decide that's worth doing, that'll be a follow-up PR, keeping this one easier to backport.
* Add list visual-diffs and dialog demos to show the sticky list controls + scroll issue * Updating Visual Diff Goldens (#3882) Co-authored-by: github-actions <[email protected]> * Fix sticky d2l-list-controls width * Updating Visual Diff Goldens (#3900) Co-authored-by: github-actions <[email protected]> * Ignore length-zero-no-unit style linting rule for css variable that will be used in calc * Avoid having to set the css variable in Javascript * Revert Avoid having to set the css variable in Javascript --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions <[email protected]> (cherry picked from commit 6b6501e)
|
🎉 This PR is included in version 2.139.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
) (This is a backport of #3899) * Add list visual-diffs and dialog demos to show the sticky list controls + scroll issue * Updating Visual Diff Goldens (#3882) Co-authored-by: github-actions <[email protected]> * Fix sticky d2l-list-controls width * Updating Visual Diff Goldens (#3900) Co-authored-by: github-actions <[email protected]> * Ignore length-zero-no-unit style linting rule for css variable that will be used in calc * Avoid having to set the css variable in Javascript * Revert Avoid having to set the css variable in Javascript --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions <[email protected]> (cherry picked from commit 6b6501e)
I tried to set this up with
vdiffs (both the old style which were super flaky and #3892), but I'm running into issues and I don't want to merge flaky tests. So for now, I've updated the demo pages for list and dialog and the list visual-diffs. dialog vdiffs will come later.The problem:
Lists jump outside of their own width 12px for their focus ring, and hover adds even a bit more with a shadow effect. We need to tell
selection-controlsto jump out as well to match, or else you can see the effects around it when scrolled:We are exposing this as a custom css property, with a default of
0ford2l-selection-controls. Thenlistuses and exposes its own property, with a default of18pxpadding. Dialog overrides this property so anyd2l-list-controlsinside will match its padding exactly (30pxdefault,20pxmobile,0pxforno-paddingmode).