Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

disable copying link when password not completed #608

Merged
merged 3 commits into from
Oct 30, 2017
Merged

Conversation

ericawright
Copy link
Contributor

@ericawright ericawright commented Oct 23, 2017

select require password, the link and "copy" button get half opacity, and the button is disabled. When the checkbox is deselected or password is submitted the opacity is restored, and button is enabled.

result:

screen shot 2017-10-23 at 9 55 59 am

Fixes: #606.
Fixes: #600.
Related: #587.

@ericawright
Copy link
Contributor Author

ericawright commented Oct 23, 2017

@clouserw can you find me a reviewer for this please?

@ghost
Copy link

ghost commented Oct 24, 2017

@fzzzy has volunteered. thanks!

@fzzzy
Copy link
Contributor

fzzzy commented Oct 27, 2017

This looks good. I'd like to figure out how to build and test this locally so I can just verify it does what I think it does.

@fzzzy
Copy link
Contributor

fzzzy commented Oct 27, 2017

This works well, but I did notice one tiny bug. I think it is mostly a nit.

To reproduce, click the copy button, then click the password checkbox while the green checkmark is still visible in the copy button. The copy button will be rendered as faded out, but it will not be disabled and the user will be able to click it to copy the url again.

I think this is because after the await delay(2000), copyBtn.disabled = false; is being executed even if the password checkbox is now checked. This is a weird corner case and it is probably fine if it doesn't get fixed.

Copy link
Contributor

@fzzzy fzzzy left a comment

Choose a reason for hiding this comment

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

Approved, with the nit mentioned in #608 (comment). If it is too inconvenient to fix at the moment, just open a new issue to document the steps to reproduce. It is a quite harmless bug.

@ericawright
Copy link
Contributor Author

ericawright commented Oct 30, 2017

nit has been fixed, thanks @fzzzy

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants