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

Adds a Clipboard Button component #1743

Merged
merged 5 commits into from
Dec 16, 2019
Merged

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented Nov 25, 2019

Fix

Closes: #232

The inline button component to copy the publish url was lacking an inidcation
that the text was copied. Additionally, the current setup was using a copy
method that can have issue in some browsers. This commit switches to use of the
clipboard npm repo, moves code into a component, and adds an indication that
the text was copied.

Test

  1. Build project
  2. Have note that is published
  3. Click copy button
  4. Observe that text is copied
  5. Observe that text of button changes to: "Copied!" for 4000ms

Review

Only one developer is required to review these changes, but anyone can perform the review.

Release

RELEASE-NOTES.txt was updated with:

  • Added indication that publish url has been copied #1743

@belcherj belcherj requested a review from a team November 25, 2019 18:13
@belcherj belcherj self-assigned this Nov 25, 2019
Copy link
Member

@codebykat codebykat left a comment

Choose a reason for hiding this comment

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

This is working well 👍

It's possible to click "Copy" before the link has been generated, we should disable that.

I did notice there's some kind of race condition if you quickly toggle the Published toggle off and on a few times... sometimes if it's off and you turn it on, it will flash the "on" state and then quickly toggle back to "off". I have reproduced that on develop as well so it wasn't introduced by this PR.

@belcherj
Copy link
Contributor Author

The button is disabled unless there is a publish url: https://github.com/Automattic/simplenote-electron/pull/1743/files#diff-5e84e31e46424a0777d56ad7fc5009b3R193

We should handle the race condition in a separate PR.

@dmsnell
Copy link
Member

dmsnell commented Nov 28, 2019

I did notice there's some kind of race condition if you quickly toggle the Published toggle off and on a few times... sometimes if it's off and you turn it on, it will flash the "on" state and then quickly toggle back to "off". I have reproduced that on develop as well so it wasn't introduced by this PR.

What's happening is that when we hit publish all we do is set a property on the note; we add a system tag saying "this note should be published" and Simperium sync's the data. When the server sees the changes going through it adds the publish links.

In between our clicks we are probably merging the local changes with the confirmation from the server of our previous clicks. If you can get the timing just right (may need to slow down your network connection) then you can probably tap a pattern into the toggle, wait a moment, and watch it mysteriously repeat the pattern back to you as the Simperium change confirmations return.

The inline button component to copy the publish url was lacking an inidcation
that the text was copied. Additionally, the current setup was using a copy
method that can have issue in some browsers. This commit switches to use of the
clipboard npm repo, moves code into a component, and adds an indication that
the text was copied.
@belcherj belcherj force-pushed the add/clipboard-button-component branch from 4c504ca to a49d69b Compare December 9, 2019 20:12
@codebykat
Copy link
Member

@belcherj I think my original comment was a bit confusing. Can we fix the button to not SEEM clickable until the note is published? Right now it still lets you click it and changes color:

Untitled

import ReactDom from 'react-dom';
import Clipboard from 'clipboard';

function ClipboardButton({ text, disabled }) {
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that we have this prop but we never set it. Should we just leave it out for now?

@codebykat
Copy link
Member

This works well for me now 👍

@dmsnell
Copy link
Member

dmsnell commented Dec 14, 2019

After writing this with React hooks how are you feeling about them @belcherj? It took me more time than usual to understand what we were trying to accomplish with them and I'm curious how you felt about thinking in terms of hooks vs. writing a traditional React class for this component.

@belcherj
Copy link
Contributor Author

belcherj commented Dec 16, 2019

Hooks are way cleaner. It may take us longer to grok now but once we are using hooks on the regular it will seem more comfortable.

@belcherj belcherj merged commit 7d3bf61 into develop Dec 16, 2019
@belcherj belcherj deleted the add/clipboard-button-component branch December 16, 2019 16:16
@codebykat codebykat added this to the 1.14 milestone Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish: Change "copy" to "copied" after click
3 participants