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.
Problem/Motivation
Settings Tray contains minor coding standards issues in it's CSS and JavaScript.
Proposed resolution
- Fix JavaScript coding standards issues.
- Add CSS cross-browser prefixes.
- Cleanup comments.
Comment | File | Size | Author |
---|---|---|---|
#22 | 2899724-22.patch | 17.6 KB | tedbow |
#20 | 2899724-20.patch | 20.77 KB | tedbow |
#11 | interdiff-2899724-7-10.txt | 1.91 KB | GrandmaGlassesRopeMan |
#10 | 2899724-10.patch | 19.66 KB | GrandmaGlassesRopeMan |
#10 | interdiff-2899724-7-10.patch | 1.91 KB | GrandmaGlassesRopeMan |
Comments
Comment #2
droplet CreditAttribution: droplet commentedComment #3
GrandmaGlassesRopeManI don't have any opinion on the CSS changes, and just some minor points on the JS fixes.
Use multi-line comment format.
Use multi-line comment format.
Use multi-line comment format.
80 char.
Comment #4
GrandmaGlassesRopeManFixed 1,2,3,4-#3
Comment #5
Wim LeersSo, #4 looks great to me.
@drpal already reviewed the JS changes that @droplet is proposing and effectively RTBC'd them.
Hopefully @DyanneNova's knowledge about Settings Tray's CSS means she can then RTBC this patch :)
Comment #6
droplet CreditAttribution: droplet commentedWelcome All CSS developers to review it. It's generic problems :)
Comment #7
DyanneNovaI don't think we should decide the font stack in this issue. I can see why the current stack was chosen, but that's probably best left up to discussion in the other issue. I agree that it would be nice to standardize where we're going for the same effect.
We shouldn't need a webkit prefix for linear gradient anymore, so I removed those lines.
Everything else looks good to me.
Comment #8
Wim LeersLet's do this then!
Comment #9
star-szrFound some minor docs things. I hope to be able to do another review in the next day or two but at a glance, the changes are looking good.
There are an awful lot of IDs in this CSS, but maybe that can be discussed on a separate issue. I'm also not sure of the history behind that, and it'd be a riskier change because the specificity would be reduced if we changed to a data attribute or class.
This @return description ("State of the…") should be indented by one more space.
/*
, should start with/**
Comment #10
GrandmaGlassesRopeMan- fixes for #9-1, 2.a, 2.b, 3, 4, 5
Comment #11
GrandmaGlassesRopeMan- actual interdiff from #10.
Comment #12
star-szrThat was quick, thanks @drpal! The changes all look great. Back to RTBC for now.
Comment #13
droplet CreditAttribution: droplet commentedThere's no history if we do the changes now :p The code was introduced from below issue:
#2826722: Add a 'fence' around settings tray with aggressive CSS reset.
I agree to do it in a separate issue
Comment #14
GrandmaGlassesRopeMan@Cottser 🙏
Comment #15
lauriiiI was trying to review this issue. The changes itself look good, but I couldn't tell if the patch fixes the issue in it's the whole scope since I don't fully understand the scope. No need for a long issue summary but I would appreciate if someone who knows more about this issue could spend few minutes writing down the scope to the issue summary.
Comment #16
droplet CreditAttribution: droplet commented(my) the scope is as simple as 3 points I listed. Just do a quick fix. -- Let's call it a QC checking :P
Comment #17
GrandmaGlassesRopeManComment #18
tedbow@drpal @droplet thanks for comments and updates. Back to RTBC
Comment #19
star-szrThanks, everyone :)
Let's leave these changes for #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways (the code changes look to be taken care of there) or #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it (a similar docs change is in the patch there).
Other than that this looks good, also this needs a reroll because of #2901205: Settings tray leaves dashed outline on followup pages, that's my fault.
Comment #20
tedbow@Cottser thanks for your on Settings Tray issues
Rerolled
Comment #21
star-szrThanks @tedbow :)
The conflicting changes I mentioned are still in the patch so this needs a reroll because I just committed #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways. Might have needed a reroll either way though.
Comment #22
tedbow@Cottser no problem! excited to get all these Settings Tray commits in!
This is reroll of #20. It has all the changes except to off-canvas.js and off-canvas.es6.js. I went through each of the changes and they were already there because of #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways which cleaned up all the js in those files.
Comment #24
star-szrI verified the changes from #20 to #22 myself, so +1 to the RTBC.
Committed and pushed 8972a66f2e to 8.5.x and 9ba27699bd to 8.4.x. Thanks!
Comment #26
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)