Problem/Motivation

from #3294005: Refactor Claro's form--password-confirm stylesheet
We need a followup issue to refactor this component in Drupal 10.0.x to make use of component-level CSS custom properties.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 Screenshot 2023-04-13 at 4.57.38 PM.png22.51 KBsantosh_verma
#5 Screenshot 2023-04-13 at 4.36.09 PM.png75.52 KBsantosh_verma
#5 Screenshot 2023-04-13 at 4.36.57 PM.png80.39 KBsantosh_verma
#5 Screenshot 2023-04-13 at 4.37.44 PM.png76.39 KBsantosh_verma
#5 Screenshot 2023-04-13 at 4.35.50 PM.png80.84 KBsantosh_verma

Issue fork drupal-3353641

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nod_ created an issue. See original summary.

Gauravvvv made their first commit to this issue’s fork.

Gauravvvv opened merge request !3822

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
santosh_verma’s picture

santosh_verma commented
StatusFileSize
new Screenshot 2023-04-13 at 4.35.50 PM.png80.84 KB
new Screenshot 2023-04-13 at 4.37.44 PM.png76.39 KB
new Screenshot 2023-04-13 at 4.36.57 PM.png80.39 KB
new Screenshot 2023-04-13 at 4.36.09 PM.png75.52 KB
new Screenshot 2023-04-13 at 4.57.38 PM.png22.51 KB

I have tested the MR #3822,
Confirm password variables now shifted into local scope from :root

Nothing broken in the UI side
UI

Week
week

Fair
Fair

Good
Good

Strong
Strong

RTBC +1

santosh_verma’s picture

santosh_verma commented
Status: Needs review » Reviewed & tested by the community
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

The MR is largely declaring variables within style blocks and using them in the following lines. These are block scoped so there's no way to override or reuse them, so these wind up being redundant and unnecessary

For example the MR

  &.is-weak {
    --password-strength-bar-weak-border-color: var(--color-maximumred);
    --password-strength-bar-weak-bg-color: var(--color-maximumred);

    border-color: var(--password-strength-bar-weak-border-color);
    background-color: var(--password-strength-bar-weak-bg-color);
  }

Provides no advantage over 

  &.is-weak {
    border-color: var(--color-maximumred);
    background-color: var(--color-maximumred);
  }
santosh_verma’s picture

santosh_verma commented

working on it

santosh_verma’s picture

santosh_verma commented
Status: Needs work » Needs review

Worked upon the suggestion given in #7, I believe that declaring variables in the styles block is redundant and unnecessary. I have addressed this suggestion in commit #8. Please take a look

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Does appear point #7 has been addressed

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs review

Posted question in the MR

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For the open thread.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed feedback from comment #12, please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

variables updated.

quietone’s picture

quietone commented

I'm triaging RTBC issues.

The issue summary doesn't have a proposed resolution but this does appear simple enough than maybe it is isn't needed here. I didn't find any unanswered questions. Leaving at RTBC.

  • lauriii committed c9453274 on 11.x 
    Issue #3353641 by Gauravvvv, Santosh_Verma, smustgrave, nod_, bnjmnm,...

  • lauriii committed 8999e680 on 10.2.x 
    Issue #3353641 by Gauravvvv, Santosh_Verma, smustgrave, nod_, bnjmnm,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed c945327 and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

lauriii closed merge request !3822

Status: Fixed » Closed (fixed)

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