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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm’s picture

Status: Active » Needs review
FileSize
35.6 KB

Decouples 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.

bnjmnm’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

lauriii’s picture

Status: Needs review » Needs work

Instead 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.

bnjmnm’s picture

Instead of overriding the JavaScript theme functions, it would be nice if we could fix regressions caused by falling back to the default module markup.

I'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

<div aria-live="polite" class="progress ajax-progress ajax-progress-bar" id="ajax-progress-edit-field-bar-0-upload-button">
	<div class="progress__label">
		&nbsp;
	</div>
	<div class="progress__track">
		<div class="progress__bar"></div>
	</div>
	<div class="progress__percentage"></div>
	<div class="progress__description">
		&nbsp;
	</div>
</div>

Ajax progress bar markup WITHOUT stable/drupal.ajax

<div class="ajax-progress ajax-progress-bar">
	<div aria-live="polite" class="progress" id="ajax-progress-edit-field-bar-0-upload-button">
		<div class="progress__label">
			&nbsp;
		</div>
		<div class="progress__track">
			<div class="progress__bar"></div>
		</div>
		<div class="progress__percentage"></div>
		<div class="progress__description">
			&nbsp;
		</div>
	</div>
</div>

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 classes ajax-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.

bnjmnm’s picture

No 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 leaves stable/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:

bnjmnm’s picture

Status: Needs work » Needs review
lauriii’s picture

I'm wondering if you have any thoughts on temporarily copying Drupal.theme.ajaxProgressBar and Drupal.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.

bnjmnm’s picture

The 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.

Status: Needs review » Needs work

The last submitted patch, 11: 3111468-11.patch, failed testing. View results

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
24.21 KB
19.83 KB

The patch in #11 did not include a few files. Interdiffing from 8 as that is the accurate change

bnjmnm’s picture

Needed 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).

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Manually 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 overrides Drupal.behaviors.password.

+++ b/core/themes/claro/claro.info.yml
@@ -119,6 +119,8 @@ libraries-extend:
+    # @todo remove bartik/temporary.ajax in https://drupal.org/node/3111468

s/bartik/claro (can be fixed on commit).

  • Gábor Hojtsy committed 4098d2d on 9.0.x
    Issue #3111468 by bnjmnm, lauriii: Decouple Stable libraries from Bartik...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Fixed #15 on commit.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.