Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We need a Front-end review for settings tray module.
Will postpone this on #2826722: Add a 'fence' around settings tray with aggressive CSS reset. which is RTBC but affects all the CSS.
But the committer that would do this review should probably also look at that issue which will probably encompass most of this review.
Issues found:
- #2896143: Unintentional animation of the body while Settings Tray is installed
- #2896143: Unintentional animation of the body while Settings Tray is installed
- #2897308: Edit toolbar tab animation is distracting
- #2897311: When there's non-zero latency, saving the Settings Tray causes the plain Toolbar to be visible first
- #2897320: Settings Tray causes the "pointer" cursor to be used both when hovering over inaccessible links (should use "default" cursor) and when in input[type=text] (should use "text" cursor)
- #2897306: Remove dead CSS
- #2897479: Settings Tray uses *both* classes *and* data- attributes, should choose one
Comments
Comment #2
tedbowComment #3
Wim LeersFront-end review question 1: Why do we need
data-off-canvas-main-canvas
?This is
outside-in-page-wrapper.html.twig
, which is a wrapper for thepage
render element, set byoutside_in_element_info_alter()
js/outside_in.es6.js
Documenting the "why" would be very valuable — it appears to be essential to the architecture of the module/JS.
Comment #4
GrandmaGlassesRopeManBrain time. I don't remember the exact discussion around #3, however the data attribute was added here, #2815831: Move Off-canvas related CSS from drupal.outside_in library to drupal.off_canvas and then referenced later in #2811717: [outsidein] Uncaught TypeError: Cannot read property 'addEventListener' of null.
Comment #5
Wim LeersTurns out that #2896143: Unintentional animation of the body while Settings Tray is installed was a simple front-end bug. A front-end review would've caught it. Already RTBC!
Comment #6
tedbow#2896960: Updated inline docs for data-off-canvas-main-canvas in twig template
Comment #7
Wim LeersFor #6.
Comment #8
Wim LeersSeveral front-end problems have been uncovered in the past few days:
Comment #9
Wim LeersSome more:
Comment #11
star-szr#2826722: Add a 'fence' around settings tray with aggressive CSS reset. is in, re-activating.
Comment #12
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)
Comment #14
Wim LeersComment #15
Wim LeersComment #16
tedbowThe 2 remaining issues
#2897308: Edit toolbar tab animation is distracting
#2897311: When there's non-zero latency, saving the Settings Tray causes the plain Toolbar to be visible first
are very minor and also would not be a problem to fix(if needed) after stable. Closing this issue