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

Copy image to clipboard #3758

Merged
merged 1 commit into from Nov 15, 2017

Conversation

Projects
None yet
5 participants
@chenba
Contributor

chenba commented Nov 13, 2017

Fix #2582

@chenba chenba requested a review from flodolo as a code owner Nov 13, 2017

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Nov 14, 2017

Collaborator

@chenba gonna pull this down and check it out now

Collaborator

johngruen commented Nov 14, 2017

@chenba gonna pull this down and check it out now

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Nov 14, 2017

Collaborator

@chenba 3 minor things:

  1. For download only mode, keep Download as the default option...(the button w/ text should be the Download button)

screen shot 2017-11-14 at 7 10 20 pm

  1. For normal mode, flip save and copy

screen shot 2017-11-14 at 7 09 35 pm

  1. The SVG fill color should match the other two...make fill: #3e3d40
Collaborator

johngruen commented Nov 14, 2017

@chenba 3 minor things:

  1. For download only mode, keep Download as the default option...(the button w/ text should be the Download button)

screen shot 2017-11-14 at 7 10 20 pm

  1. For normal mode, flip save and copy

screen shot 2017-11-14 at 7 09 35 pm

  1. The SVG fill color should match the other two...make fill: #3e3d40
@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Nov 14, 2017

Collaborator

ALSO...left need a string change on the message

Collaborator

johngruen commented Nov 14, 2017

ALSO...left need a string change on the message

@chenba

This comment has been minimized.

Show comment
Hide comment
@chenba

chenba Nov 14, 2017

Contributor

@johngruen Need to confirm: do you want to swap the download and copy buttons only for download-only mode? Or, we keep them consistent, always Cancel, Copy, Download [, Save]?

Contributor

chenba commented Nov 14, 2017

@johngruen Need to confirm: do you want to swap the download and copy buttons only for download-only mode? Or, we keep them consistent, always Cancel, Copy, Download [, Save]?

@chenba

This comment has been minimized.

Show comment
Hide comment
@chenba

chenba Nov 14, 2017

Contributor

ALSO...left need a string change on the message

@johngruen I don't understand this comment. Left?

Contributor

chenba commented Nov 14, 2017

ALSO...left need a string change on the message

@johngruen I don't understand this comment. Left?

@chenba

This comment has been minimized.

Show comment
Hide comment
@chenba

chenba Nov 14, 2017

Contributor

@johngruen Swapped the position of the copy and download buttons. Take another look?

Contributor

chenba commented Nov 14, 2017

@johngruen Swapped the position of the copy and download buttons. Take another look?

@ianb

This comment has been minimized.

Show comment
Hide comment
@ianb

ianb Nov 14, 2017

Contributor

The code looks good, though for some reason the translations aren't working for me. I've even done a make clean. The translations are obviously there, but I get warnings like Unknown localization message copyscreenshot

That might just be a caching problem? @6a68: have you seen that before?

Contributor

ianb commented Nov 14, 2017

The code looks good, though for some reason the translations aren't working for me. I've even done a make clean. The translations are obviously there, but I get warnings like Unknown localization message copyscreenshot

That might just be a caching problem? @6a68: have you seen that before?

@chenba

This comment has been minimized.

Show comment
Hide comment
@chenba

chenba Nov 14, 2017

Contributor

@ianb Yeah, this is related to #3661. If you load the addon through about:debugging then you'll definitely see the new l10n messages.

Contributor

chenba commented Nov 14, 2017

@ianb Yeah, this is related to #3661. If you load the addon through about:debugging then you'll definitely see the new l10n messages.

@ianb

This comment has been minimized.

Show comment
Hide comment
@ianb

ianb Nov 14, 2017

Contributor

OK, we can merge (should also get squashed) once @johngruen gives his 👍

Contributor

ianb commented Nov 14, 2017

OK, we can merge (should also get squashed) once @johngruen gives his 👍

@johngruen

It should be Cancel > Copy > Download > Save (if applicable) w/ the rightmost option having both icon and copy

Show outdated Hide outdated locales/en-US/webextension.properties
Show outdated Hide outdated locales/en-US/webextension.properties
@chenba

This comment has been minimized.

Show comment
Hide comment
@chenba

chenba Nov 15, 2017

Contributor

@flodolo Updated a couple of l10n messages in the latest commit (8e33cb7). Could you take a quick look?

Contributor

chenba commented Nov 15, 2017

@flodolo Updated a couple of l10n messages in the latest commit (8e33cb7). Could you take a quick look?

@flodolo

Strings look good

@chenba chenba merged commit 41e27fd into mozilla-services:master Nov 15, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@chenba chenba deleted the chenba:2582-copy-to-clipboard branch Nov 15, 2017

@eyaler

This comment has been minimized.

Show comment
Hide comment
@eyaler

eyaler Nov 16, 2017

is there a copy button also in the my shots library?
how about binding with ctrl-c?

eyaler commented Nov 16, 2017

is there a copy button also in the my shots library?
how about binding with ctrl-c?

@chenba

This comment has been minimized.

Show comment
Hide comment
@chenba

chenba Nov 16, 2017

Contributor

@eyaler ctrl-c is in this patch. There's no copy (to clipboard) button in My Shots; an issue's been filed for adding one to the page of an individual shot (#1646). Could you file one for My Shots?

Contributor

chenba commented Nov 16, 2017

@eyaler ctrl-c is in this patch. There's no copy (to clipboard) button in My Shots; an issue's been filed for adding one to the page of an individual shot (#1646). Could you file one for My Shots?

@eyaler

This comment has been minimized.

Show comment
Hide comment
@eyaler

eyaler commented Nov 16, 2017

here: #3792

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