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.

Sign up for GitHub

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

Jump to bottom

Minor improvements for the permalink Copy to clipboard button #6472

Merged
merged 2 commits into from
Apr 30, 2018
Merged

Minor improvements for the permalink Copy to clipboard button #6472

merged 2 commits into from
Apr 30, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Apr 27, 2018

Waiting for #5494, I'd like to propose a few minor improvements for the permalink "Copy" button.

This PR tries to re-use existing component and patterns. Specifically:

  • uses an IconButton component instead of a Button
  • uses classnames() to conditionally apply the is-copied CSS class
  • shortens the label to 'Copy the permalink'
  • makes the button style more similar to default icon buttons (see border-radius)
  • improves the button alignment

Screenshot before:

before

After:

after

after2

@afercia afercia requested a review from youknowriad April 27, 2018 18:05
youknowriad
youknowriad reviewed Apr 30, 2018
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Nice improvement, maybe we should hide the borders completely when the button has the is-copied class. It feels a bit strange to be able to hover it even if disabled.

@afercia
Copy link
Contributor Author

afercia commented Apr 30, 2018 •
edited
Loading

@youknowriad thanks. In the latest commit:

  • the simpler way to reset the styles when the permalink is copied is to "disable" the button: the CSS already provided some styles for disabled buttons on hover, I've applied the same to focus
  • to avoid a focus loss, I'm using aria-disabled instead of disabled.
  • updated the label/tooltip when the permalink is copied

Question: should the button "noop" when the permalink is already copied? Not sure...
After all, I'm not sure "disabling" the button is a good idea in the first place. For example, if I copy the permalink, then I edit the permalink and I want to copy it again before saving the draft or updating the post, I can't do that: the button still looks "disabled"

TIL: updating the tooltip to empty string or null produces a focus loss, I guess because the whole parent + children get re-rendered.

youknowriad
youknowriad reviewed Apr 30, 2018
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Maybe we shouldn't really disable the button but make it look disabled. I'm asking because right now when I copy, I don't see the tooltip "Permalink copied" I have to move away from the button and hover back.

@afercia
Copy link
Contributor Author

afercia commented Apr 30, 2018

Maybe we shouldn't really disable the button

@youknowriad it's not disabled :)

when I copy, I don't see the tooltip "Permalink copied" I have to move away from the button and hover back.

that's just how the tooltip works on any button: it disappears when clicking, also on master.

@youknowriad
Copy link
Contributor

I guess we should leave the tooltip fix for another PR in that case :)

youknowriad
youknowriad approved these changes Apr 30, 2018
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@afercia
Copy link
Contributor Author

afercia commented Apr 30, 2018

Thanks!

@afercia afercia merged commit 414d850 into master Apr 30, 2018
@afercia afercia deleted the update/permalink-copy-button-minor-improvements branch April 30, 2018 13:09
@danielbachhuber danielbachhuber added this to the 2.8 milestone Apr 30, 2018
aduth
aduth reviewed May 1, 2018
View reviewed changes
components/clipboard-button/index.js
@@ -82,9 +82,9 @@ class ClipboardButton extends Component {

return (
<span ref={ this.bindContainer }>
<Button { ...buttonProps } className={ classes }>
<IconButton { ...buttonProps } icon="admin-links" className={ classes }>
Copy link
Member

Choose a reason for hiding this comment

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

Should this have been applied to ClipboardButton broadly, or is it specific to PostPermalink ?

It's affecting other usage, such as the ClipboardButtons shown in the editor crash prompt:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I missed that sorry :(

@afercia
Copy link
Contributor Author

afercia commented May 2, 2018

Will have a look.

@afercia afercia mentioned this pull request May 2, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@youknowriad youknowriad youknowriad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.8
Development

Successfully merging this pull request may close these issues.
4 participants
@afercia @youknowriad @aduth @danielbachhuber