Skip to content

LG-9208: Confirm password#8161

Closed
jc-gsa wants to merge 8 commits intomainfrom
LG-9208-confirm-password
Closed

LG-9208: Confirm password#8161
jc-gsa wants to merge 8 commits intomainfrom
LG-9208-confirm-password

Conversation

@jc-gsa
Copy link
Contributor

@jc-gsa jc-gsa commented Apr 10, 2023

No description provided.

Comment on lines +16 to +17
name: :password_confirmation,
type: :password_confirmation,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will need to be feature flagged in some manner since it will be backwards incompatible when running alongside a version that does not require password_confirmation.

**tag_options
)
@form = form
@label = label
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the assigned value here may not exist in scope. Should it be a constructor argument?

Comment on lines +4 to +12
"version": "1.0.0",
"peerDependencies": {
"react": "^17.0.2"
},
"peerDependenciesMeta": {
"react": {
"optional": true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't appear that we're using React in the package.

Suggested change
"version": "1.0.0",
"peerDependencies": {
"react": "^17.0.2"
},
"peerDependenciesMeta": {
"react": {
"optional": true
}
}
"version": "1.0.0"

/**
* Text or password confirmation input.
*/
get input_confirmation(): HTMLInputElement {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use camel-case in JavaScript (typically I'd expect the linter to flag this, but I don't know if it applies to class function names).

Suggested change
get input_confirmation(): HTMLInputElement {
get inputConfirmation(): HTMLInputElement {

Comment on lines +23 to +30
return this.querySelector('.password-confirmation__input1')! as HTMLInputElement;
}

/**
* Text or password confirmation input.
*/
get input_confirmation(): HTMLInputElement {
return this.querySelector('.password-confirmation__input2')! as HTMLInputElement;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're calling these "input" and "input confirmation" in the code, maybe we could represent the class names similarly for consistency?

Suggested change
return this.querySelector('.password-confirmation__input1')! as HTMLInputElement;
}
/**
* Text or password confirmation input.
*/
get input_confirmation(): HTMLInputElement {
return this.querySelector('.password-confirmation__input2')! as HTMLInputElement;
return this.querySelector('.password-confirmation__input')! as HTMLInputElement;
}
/**
* Text or password confirmation input.
*/
get input_confirmation(): HTMLInputElement {
return this.querySelector('.password-confirmation__input-confirmation')! as HTMLInputElement;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect we're going to hit some conflicts in this file between this and #8085, just a heads-up between you and @jmdembe .

@jc-gsa jc-gsa closed this Apr 12, 2023
@aduth aduth mentioned this pull request Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants