Problem/Motivation
As stated in #3110855: Plan for removing dependency to Stable in Bartik/Seven/Claro/Umami, Bartik/Seven/Claro/Umami will be decoupled from Stable. This includes the two libraries provided by Stable, which extend existing core libraries.
drupal.ajax:
version: VERSION
js:
js/ajax.js: {}
drupal.user:
version: VERSION
js:
js/user.theme.js: {}
libraries-extend:
core/drupal.ajax:
- stable/drupal.ajax
user/drupal.user:
- stable/drupal.user
stable/drupal.ajax added in #3059847: Move hard coded AJAX progress bar classes to a theme function
stable/drupal.user added in #3061265: Use specific class for password confirm message
Proposed resolution
Confirm what the library provides for each theme, determine if it is still necessary. If yes, transfer the functionality to the core themes that need it in the most easy-to-maintain way possible.
Remaining tasks
Determine which libraries are needed by each theme
No themes require the functionality provided by stable/drupal.ajax
.
Bartik, Seven. and Umami need to modify user.css to receive the styling that was previously made available by the class added in stable/drupal.user
Update classy/components/user.css in Bartik, Seven. and Umami so stable/drupal.user
isn't needed. Move to theme specific library as it no longer matches classy.
Update ConfirmClassyCopiesTest to reflect the changes to user.css
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#14 | 3111468-14-reroll.patch | 22.8 KB | bnjmnm |
#13 | interdiff_8-13.txt | 19.83 KB | bnjmnm |
#13 | 3111468-13.patch | 24.21 KB | bnjmnm |
#11 | 3111468-11.patch | 19.74 KB | bnjmnm |
#11 | interdiff_8-11.txt | 15.36 KB | bnjmnm |
Comments
Comment #2
bnjmnmComment #3
bnjmnmDecouples libraries and adds tests based on ThemeNotUsingClassyLibraryTest.
Both libraries are for BC support but have not yet been deprecated so unsure which branches this would be applied to.
Comment #4
bnjmnmComment #5
Gábor HojtsyComment #6
lauriiiInstead of overriding the JavaScript theme functions, it would be nice if we could fix regressions caused by falling back to the default module markup. Both of the changes are minor markup changes that should be easy for us to test. This way we don't have to continue to maintain this code and we can benefit from the improvements these are covering.
Comment #7
bnjmnmI'm pleased to hear this is an option! Perhaps backwards-compatibility-fatigue got in the way of me seeing it as a feasible one.
I'm of the opinion that
stable/drupal.ajax
is not necessary in any of the core themes.I compared the markup in each core theme with and without
stable/drupal.ajax
. Note that the markup is the same in each theme so no theme-specific examples are needed.Ajax progress bar markup WITH stable/drupal.ajax
Ajax progress bar markup WITHOUT stable/drupal.ajax
This confirms the only difference is from #3059847: Move hard coded AJAX progress bar classes to a theme function
The backwards compatibility provided by
stable/drupal.ajax
is not required by any of the core themes. Manual testing confirmed that moving the classesajax-progress ajax-progress-bar
to a wrapper div has no visual or functional difference.Tagging with needs issue summary update - which will be addressed once I've determined if
stable/drupal.user
is necessary.Comment #8
bnjmnmNo interdiff as this is a fully different approach from earlier patches.
After determining
stable/drupal.ajax
does not need to be addressed in core themes, that leavesstable/drupal.user
, which needs to be addressed in Bartik, Seven and Umami. This was a BC layer added in #3061265: Use specific class for password confirm message, where the password confirm message class was changed.. Without the stable library, the core themes should use the new class. The specific functionality to look for is that the text following "Passwords match:" in password confirm messages should be green/red depending on the match status.For now, the css rules have been expanded to cover both selectors, with a @toto to remove the old one once Stable is no longer a base theme in #3115223: Remove Stable as a base theme of core themes
Here's how it looks with the manual testing patch:
Comment #9
bnjmnmComment #10
lauriiiI'm wondering if you have any thoughts on temporarily copying
Drupal.theme.ajaxProgressBar
andDrupal.theme.passwordConfirmMessage
theme functions from modules to themes? We could marking them with a @todo to be removed as part of #3115223: Remove Stable as a base theme of core themes. The reason I would like that approach, is that this would ensure that the code continues to work with future changes. This would also give us some more time to catch potential regressions.Comment #11
bnjmnmThe suggestion in #10 is a good one. Without that, it's incredibly unlikely that any regressions would be caught, and it makes things difficult to test. It is a good use of beta release status. Applying this change required more steps than anticipated, but I still think it's a good idea.
Comment #13
bnjmnmThe patch in #11 did not include a few files. Interdiffing from 8 as that is the accurate change
Comment #14
bnjmnmNeeded to reroll after #3115088: Remove Classy as a base theme of core themes
After the reroll, quite a bit of this patch is for the temporary inclusion of theme function as suggested in #10. I still think this is a good idea -- but if those specific changes are slowing this down it may be worth skipping as there's only one issue other than this one that needs to land before core themes don't need to declare Stable as a base theme: #3113211: Create test for theme asset decoupling from Stable and address any regressions that decoupling may cause
Once that issue and this one is done, #3115223: Remove Stable as a base theme of core themes can be reviewed (in that issue, there's already a patch to get all the tests passing, so things can potentially move quickly).
Comment #15
lauriiiManually tested all themes (Bartik, Umami, Seven and Claro) in both scenarios, the user password confirm message and progress bar and there wasn't any regressions. I also confirmed that we don't have to override
Drupal.theme.passwordConfirmMessage
in Claro because it overridesDrupal.behaviors.password
.s/bartik/claro (can be fixed on commit).
Comment #17
Gábor HojtsyThanks! Fixed #15 on commit.