Help user to choose a valid password in the reset password input #819
Conversation
Steffengreiner
changed the title
There was a problem hiding this comment.
The error messages should be the same. In the registration case, the box does not show a helper and shows the helper "xyz characters are required..." when the input is not valid. The helper disappears with valid input. In the password reset form, the helper text is always shown and password to short is shown when not enough characters are provided. Is this the intended design @Shraddha0903 ?
| * | ||
| * @since 1.0.0 | ||
| */ | ||
| public class BoxLayout extends VerticalLayout { |
There was a problem hiding this comment.
How is this change related to the issue? What happened here and why?
There was a problem hiding this comment.
The BoxLayout was a remnant from the time in which we relied on the Vaadin specified styling(Vertical Layout). This change aims to harmonize on how the login, registration and password related components can be harmonized in our style format. (CSS based styling)
| * Card styled Layout employed within all {@link Main} in the | ||
| * {@link life.qbic.datamanager.views.login.LoginLayout} | ||
| */ | ||
| public class CardLayout extends Div { |
There was a problem hiding this comment.
What is a Card Layout? where is it used? How can it look. I am confused about this.
There was a problem hiding this comment.
I updated the JD for more clarity. In a nutshell we currently employ the same card-like style for all components during password reset, registration and login. To avoid having to copy paste the css over and over again, this DIV based component contains the style necessary to make it look like a hovering card containing the input fields.
There was a problem hiding this comment.
I updated the JD for more clarity. In a nutshell we currently employ the same card-like style for all components during password reset, registration and login. To avoid having to copy paste the css over and over again, this DIV based component contains the style necessary to make it look like a hovering card containing the input fields.
You do not need to copy paste anything but the css class name if I am not mistaken? I would vote in favor of composition over inheritance here. Components that extend card cannot extend anything else. If you want it to look like a card, why not only add the css class name. Shouldn't this be sufficient?
There was a problem hiding this comment.
Good point! For me it feels more like a feeling thing, we want to reuse the same styling for all the components in this context, so i'd like to have it as a general java class. But since it provides no functionality as of yet, we can also go for YAGNI and copy paste the css class. I adapted the PR accordingly and removed the cardlayout 🤔
| */ | ||
| @SpringComponent | ||
| @UIScope | ||
| public class ResetEmailSentComponent extends CardLayout { |
There was a problem hiding this comment.
why not make this a page instead and rout to it from the reset page?
There was a problem hiding this comment.
That's a good question.
Currently there is not a real benefit to seperate this functionality into a different page, since the user will only be shown the reset email sent component if he triggered the process during the password reset main. Do you think there is a need to seperate this functionality into different routes?
There was a problem hiding this comment.
I think that this information has nothing to do with the reset form. No strong feelings just feels like an independent informational page.
There was a problem hiding this comment.
I'd like to keep it like this for now if that's ok. Down the line if the need arises, it's easily changed to a seperate page 👍
| */ | ||
| @SpringComponent | ||
| @UIScope | ||
| public class ResetPasswordComponent extends CardLayout { |
There was a problem hiding this comment.
According to the javadoc this component and NewPasswordSetComponent have the same purpose?
There was a problem hiding this comment.
Nice catch 👁️. I updated the JD to better reflect their purpose.
(Reset Password --> Inform user about new password set)
user-interface/src/main/java/life/qbic/datamanager/views/register/RegistrationOrcIdMain.java
Outdated
Show resolved
Hide resolved
| @@ -35,7 +35,7 @@ | |||
| */ | |||
| @SpringComponent | |||
| @UIScope | |||
| public class UserRegistrationComponent extends Div { | |||
| public class UserRegistrationComponent extends CardLayout { | |||
There was a problem hiding this comment.
The Cardlayout is a DIV Container with a specific style. This style is the same for all components related to password reset, user registration and login. Therefore it's easier to maintain if we have one java class which defines the style and reuse it in the different main classes
user-interface/src/main/java/life/qbic/datamanager/views/register/UserRegistrationMain.java
Outdated
Show resolved
Hide resolved
…in SetNewPasswordComponent
…g registration and setting a new password
@KochTobi Hmm i see the issue here, the text is redundant in the first case and the second case provides no information beforehand. |
|
It seems that this was not addressed, yet? Otherwise the PR is good to go imo. |
Steffengreiner
dismissed
KochTobi’s stale review
Andreas took over, thanks for the feedback <3
Steffengreiner
deleted the
feature/dm-703-overhaul-password-reset-functionality
branch
Transfers the changes made #716 to the dev branch state after the reset
What was changed
Introduce current concept of main components and remove vaadin based components from rest password and new password sending components.
Additionally provide binders checking if the provided password and email is valid