Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new vue components in login form #33781

Merged
merged 2 commits into from Sep 6, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Aug 31, 2022

Close #33822

Error handling:

image

@juliushaertl
Copy link
Member

master now has the vue components bump :)

- Improve accessibility
- Simply code

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan force-pushed the port/login-form/vue-password-components branch 2 times, most recently from 37b8615 to c12c5ce Compare September 5, 2022 09:56
@CarlSchwan CarlSchwan marked this pull request as ready for review September 5, 2022 10:00
@CarlSchwan CarlSchwan requested review from a team, PVince81, Pytal and szaimen and removed request for a team September 5, 2022 10:00
@CarlSchwan CarlSchwan self-assigned this Sep 5, 2022
@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Sep 5, 2022
@CarlSchwan CarlSchwan force-pushed the port/login-form/vue-password-components branch from c12c5ce to 292bd8d Compare September 5, 2022 11:05
Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Looks good design wise!
Maybe this is irrelevant to this PR but 2 primary buttons close to each other is generally avoided, instead the Login with Keycloak button can have secondary button styling.

@CarlSchwan
Copy link
Member Author

Looks good design wise! Maybe this is irrelevant to this PR but 2 primary buttons close to each other is generally avoided, instead the Login with Keycloak button can have secondary button styling.

Good idea, it looks indeed nicer :)

image

@CarlSchwan CarlSchwan force-pushed the port/login-form/vue-password-components branch from 292bd8d to bbd0d42 Compare September 6, 2022 07:47
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Looks good :)

@CarlSchwan CarlSchwan force-pushed the port/login-form/vue-password-components branch from bbd0d42 to 1351ad1 Compare September 6, 2022 09:07
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Very nice! Just 2 details, possibly separate issues from this:

  • Wording-wise we want to get away from saying "user" so should be "account name or email" here

  • Not sure if it was introduced with this, but container has more space to the top than to the left

- Move css in scopped vue components
- Port to NcNoteCard all the warning messages

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan force-pushed the port/login-form/vue-password-components branch from 1351ad1 to 43d1aa3 Compare September 6, 2022 13:20
@CarlSchwan
Copy link
Member Author

Very nice! Just 2 details, possibly separate issues from this:

* Wording-wise we want to get away from saying "user" so should be "account name or email" here

* Not sure if it was introduced with this, but container has more space to the top than to the left

Done, new screenshot :)

image

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Nice! :)

@CarlSchwan CarlSchwan merged commit 36b2d3d into master Sep 6, 2022
@CarlSchwan CarlSchwan deleted the port/login-form/vue-password-components branch September 6, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Password visibility icon in login page out of place
6 participants