Skip to content

Comments

adding toggle password to be able to see it when login#15881

Merged
rullzer merged 1 commit intonextcloud:masterfrom
PhieF:toggle-password-login
Jul 9, 2019
Merged

adding toggle password to be able to see it when login#15881
rullzer merged 1 commit intonextcloud:masterfrom
PhieF:toggle-password-login

Conversation

@PhieF
Copy link
Contributor

@PhieF PhieF commented Jun 5, 2019

Hi

Here is a preview of what it does

Capture d’écran de 2019-06-05 14-42-51

Capture d’écran de 2019-06-05 14-42-57

What do you think of that ? If approved, I will also push it to registration

@kesselb kesselb added 3. to review Waiting for reviews enhancement labels Jun 5, 2019
@kesselb kesselb added this to the Nextcloud 17 milestone Jun 5, 2019
skjnldsv
skjnldsv reviewed Jun 5, 2019
View reviewed changes
skjnldsv
skjnldsv requested changes Jun 5, 2019
View reviewed changes
@skjnldsv skjnldsv requested a review from juliusknorr June 5, 2019 13:20
@skjnldsv
Copy link
Member

skjnldsv commented Jun 5, 2019

Hello @PhieF !!
Thanks for your request! This is a great work :)

I commented a few nitpicks! ✌️

skjnldsv
skjnldsv approved these changes Jun 5, 2019
View reviewed changes
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Awesome!
Please rebase and squash your commits into one and don't forget to add the compiled files as well :)

@PhieF PhieF force-pushed the toggle-password-login branch from 989d98d to a38ad01 Compare June 5, 2019 15:20
@PhieF
Copy link
Contributor Author

PhieF commented Jun 5, 2019

I guess it should be ok, I generated .js files just with a make command, is it ok ? make build-js was creating large js files instead of single lines ones

@skjnldsv
Copy link
Member

skjnldsv commented Jun 5, 2019 •
edited
Loading

@PhieF make is perfect. It create everything from scratch (npm install and build-js-production).
make build-js-production is the command to only build the js files for production, so it's ok as well :)
make build-js is to build the development files :)

@skjnldsv
Copy link
Member

skjnldsv commented Jun 6, 2019

@PhieF it seems that you did not signed off your work :)
https://github.com/nextcloud/server/pull/15881/checks?check_run_id=142407417

@PhieF PhieF force-pushed the toggle-password-login branch from a38ad01 to f21336b Compare June 6, 2019 14:22
@PhieF
Copy link
Contributor Author

PhieF commented Jun 6, 2019

Done !

@skjnldsv
Copy link
Member

skjnldsv commented Jun 6, 2019

@nextcloud/designers you're up! 👀

georgehrke
georgehrke approved these changes Jun 6, 2019
View reviewed changes
Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

Looks really good 👍

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 6, 2019
jancborchardt
jancborchardt requested changes Jun 11, 2019
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good stuff @PhieF! Some design & accessibility issues:

  • The clickable area of the toggle should be at least 44*44px as per our design guidelines. Ideally it reaches the full height (top & bottom) of the input field, and also all the way to the right of the input field so people don’t have to tap exactly.
  • The toggle needs to have a hover&focus state (full opacity of the icon) for proper feedback on hover and keyboard focus
  • The toggle is missing an aria label to describe it, like aria-label="Show password"
  • The img needs to have an alt="" to define that the image is decorative (if you add the aforementioned aria label

@skjnldsv skjnldsv added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Jun 11, 2019
@kesselb kesselb mentioned this pull request Jun 15, 2019
Closed
@rullzer
Copy link
Member

rullzer commented Jun 25, 2019

@skjnldsv @ChristophWurst could you help out here to get the comments by @jancborchardt resolved so we can get this in?

@jancborchardt jancborchardt mentioned this pull request Jun 26, 2019
Closed
77 tasks
@rullzer rullzer force-pushed the toggle-password-login branch from f21336b to a70069b Compare July 5, 2019 15:11
@rullzer rullzer force-pushed the toggle-password-login branch from a70069b to 8fa2d70 Compare July 9, 2019 08:54
@rullzer
Copy link
Member

rullzer commented Jul 9, 2019

Lets merge this and focus on the other points while polishing

@rullzer rullzer merged commit 22de685 into nextcloud:master Jul 9, 2019
@welcome
Copy link

welcome bot commented Jul 9, 2019

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jancborchardt jancborchardt jancborchardt requested changes

@georgehrke georgehrke georgehrke approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

No one assigned

Labels

2. developing Work in progress enhancement

Projects

None yet

Milestone

Nextcloud 17

Development

Successfully merging this pull request may close these issues.

7 participants

@PhieF @skjnldsv @rullzer @jancborchardt @georgehrke @ChristophWurst @kesselb