Conversation
b31878e
to
389aaf5
Compare
addonDescription = Firefox Screenshots takes clips and screenshots from pages, and can save a permanent copy of a page. | ||
addonAuthorsList = Ian Bicking, Donovan Preston, and Bram Pitoyo | ||
addonDescription = Firefox Screenshots takes clips and screenshots from the Web and lets you save them temporarily or permanently. | ||
addonAuthorsList = Ian Bicking, Donovan Preston, Jared Hirsch, Danny Coates, Morpheus Chen, Helen Huang, Mark Liang, Fang Shih, John Gruen, Bram Pitoyo, Sharon Bautista and Sean Martell, Michelle Heubusch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just Mozilla <https://mozilla.org>
, like we do with Test Pilot's package.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use screenshots-feedback@mozilla.org (which exists)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that could work
myShotsLink = My Shots | ||
screenshotInstructions = Drag or click on the page to select a region. Press ESC to cancel. | ||
screenshotInstructionOne = Drag or click on the page to select a region. | ||
screenshotIntrouctionTwo = Press ESC to cancel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh also, the template that renders this string will have to get updated to include both keys.
connectionErrorTitle = We can’t connect to the Firefox Screenshots server. | ||
connectionErrorDetails = Please check your network connection. If you are able to connect to the Internet, there may be a temporary problem with the Firefox Screenshots service. | ||
loginErrorDetails = We couldn’t save your shot because we couldn’t access your account on the Firefox Screenshots server. | ||
loginConnectionErrorDetails = Please check your network connection. If you are able to connect to the Internet, there may be a temporary problem with the Firefox Screenshots service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking we would not keep the separate string ID if the string itself is identical to connectionErrorDetails
, it's wasted work for the translators. @flodolo, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it doesn't make a lot of sense to have two separate IDs for the same string. connectionErrorDetails
sounds good for both.
loginErrorDetails = We couldn’t save your shot because we couldn’t access your account on the Firefox Screenshots server. | ||
loginConnectionErrorDetails = Please check your network connection. If you are able to connect to the Internet, there may be a temporary problem with the Firefox Screenshots service. | ||
unshootablePageErrorTitle = We can’t screenshot this page. | ||
unshootablePageErrorDetails = his isn’t a standard Web page, so Firefox Screenshots can’t take a screenshot of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: This
, not his
@johngruen The new strings look good; I added some comments. The /webextension/* files look like build output, so they should be removed. |
connectionErrorTitle = We can’t connect to the Firefox Screenshots server. | ||
connectionErrorDetails = Please check your network connection. If you are able to connect to the Internet, there may be a temporary problem with the Firefox Screenshots service. | ||
loginErrorDetails = We couldn’t save your shot because we couldn’t access your account on the Firefox Screenshots server. | ||
loginConnectionErrorDetails = Please check your network connection. If you are able to connect to the Internet, there may be a temporary problem with the Firefox Screenshots service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it doesn't make a lot of sense to have two separate IDs for the same string. connectionErrorDetails
sounds good for both.
genericErrorDetails = Try again or take a shot on another page? | ||
genericErrorTitle = Whoa! Screenshots went haywire. | ||
genericErrorDetails = We’re not sure what just happened. Care to try again or take a shot of a different page? | ||
## Onboarding Strings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One # is enough
tourHeaderOneSuper = Beta | ||
tourBodyOne = Take, save, and share screenshots without leaving Firefox. | ||
tourHeaderTwo = Capture Just What You Want | ||
tourBodyTwo = Click and drag to capture just a portion of a page. You can also hover to highlight your your selection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No double spaces after periods, double your
tourBodyThree = Save your cropped shots to the Web for easier sharing, or download them to your computer. You also can click on the My Shots button to find all the shots you’ve taken. | ||
tourHeaderFour = Capture Windows or Entire Pages | ||
tourBodyFour = Select the buttons in the upper right to capture the visible area in the window or to capture an entire page. Shot Taken Push Notification | ||
>>>>>>> provisional addon strings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Git merge conflict leftover?
tourHeaderThree = As You Like it | ||
tourBodyThree = Save your cropped shots to the Web for easier sharing, or download them to your computer. You also can click on the My Shots button to find all the shots you’ve taken. | ||
tourHeaderFour = Capture Windows or Entire Pages | ||
tourBodyFour = Select the buttons in the upper right to capture the visible area in the window or to capture an entire page. Shot Taken Push Notification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's Shot Taken Push Notification
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty annoying that github showed all these typos as fixed. Going through them now.
this is what happens when i try to do stuff while in a meeting |
389aaf5
to
5e7218c
Compare
a7daca7
to
333622c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still one issue to fix, but string-wise it look good.
P.S. @Flod is not the droid you're looking for ;-)
tourHeaderOneSuper = Beta | ||
tourBodyOne = Take, save, and share screenshots without leaving Firefox. | ||
tourHeaderTwo = Capture Just What You Want | ||
tourBodyTwo = Click and drag to capture just a portion of a page. You can also hover to highlight your your selection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still double your at the end
333622c
to
9577ef4
Compare
Thanks! |
addon/webextension/selector/ui.js
Outdated
@@ -222,7 +225,8 @@ window.ui = (function () { // eslint-disable-line no-unused-vars | |||
<div class="full-page"></div> | |||
</div> | |||
`; | |||
this.el.querySelector(".preview-instructions").textContent = browser.i18n.getMessage("screenshotInstructions"); | |||
this.el.querySelector(".preview-instructions-start").textContent = browser.i18n.getMessage("previewInstructionsStart"); | |||
this.el.querySelector(".preview-instructions-end").textContent = browser.i18n.getMessage("previewInstructionsEnd"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny indentation?
9577ef4
to
e433a21
Compare
@johngruen No worries, I figured you were doing three other things at the same time 👍 I'll take another look and rebase this branch real quick |
7541de2
to
9419640
Compare
@johngruen I added the remaining fixes, so you are off the hook here, except for one question: the string We can do another PR if any last-minute changes arrive tomorrow, then I'll bump the bugzilla bug to get localizers started. |
addon/webextension/selector/ui.js
Outdated
<div class="preview-instructions"> | ||
<span class="preview-instructions-step-one"></span> | ||
| ||
<span class="preview-instructions-step-two"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @stasm (or @flodolo), I'm not sure if this is the right way to put two strings on the same visual line such that it renders correctly in RTL languages: span
+
+ span
, and setting the textContent
of each span
to a different localized string.
Here's how this looks in regular English:
If I set dir="rtl"
in the body
tag on the iframe, English RTL looks like this:
Does that look correct?
Finally, the one annoyance about this approach is that, when the screen is narrow, the strings wrap a bit awkwardly:
Any suggestions there? I could concatenate the strings and shove them into a single element, but wasn't sure that would work for RTL languages. @johngruen may have ideas for nicer wrapping in that situation, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The English result with forced RTL direction is expected, period is a weak character and follows RTL, the rest is LTR.
The real question is, why do we need to split this string in two? Can't we just use one? That would also solve the awkward wrapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flodolo Sure, great point. I'll reconstruct the string.
I'll wait to merge this till I get feedback on the string concatenation approach in the selector overlay, but I think it's otherwise ready. |
3234cf2
to
461376c
Compare
461376c
to
65ab3e2
Compare
@6a68 this is still provisional but probably worth merging. I kind of want to review some of the old keys b/c they're not very didactic.