Skip to content
This repository has been archived by the owner. It is now read-only.

provisional addon strings #2448

Merged
merged 1 commit into from Mar 23, 2017
Merged

provisional addon strings #2448

merged 1 commit into from Mar 23, 2017

Conversation

@johngruen
Copy link
Contributor

@johngruen johngruen commented Mar 21, 2017

@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.

@johngruen johngruen force-pushed the provisional-addon-strings branch from b31878e to 389aaf5 Mar 21, 2017
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
Copy link
Member

@jaredhirsch jaredhirsch Mar 21, 2017

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?

Copy link
Contributor

@ianb ianb Mar 21, 2017

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)

Copy link
Contributor Author

@johngruen johngruen Mar 21, 2017

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.
Copy link
Member

@jaredhirsch jaredhirsch Mar 21, 2017

Choose a reason for hiding this comment

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

typo

Copy link
Member

@jaredhirsch jaredhirsch Mar 21, 2017

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.
Copy link
Member

@jaredhirsch jaredhirsch Mar 21, 2017

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?

Copy link
Collaborator

@flodolo flodolo Mar 22, 2017

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.
Copy link
Member

@jaredhirsch jaredhirsch Mar 21, 2017

Choose a reason for hiding this comment

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

typo: This, not his

@jaredhirsch
Copy link
Member

@jaredhirsch jaredhirsch commented Mar 21, 2017

@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.
Copy link
Collaborator

@flodolo flodolo Mar 22, 2017

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
Copy link
Collaborator

@flodolo flodolo Mar 22, 2017

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.
Copy link
Collaborator

@flodolo flodolo Mar 22, 2017

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
Copy link
Collaborator

@flodolo flodolo Mar 22, 2017

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
Copy link
Collaborator

@flodolo flodolo Mar 22, 2017

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?

Copy link
Member

@jaredhirsch jaredhirsch Mar 22, 2017

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.

@johngruen
Copy link
Contributor Author

@johngruen johngruen commented Mar 22, 2017

this is what happens when i try to do stuff while in a meeting

@johngruen johngruen force-pushed the provisional-addon-strings branch from 389aaf5 to 5e7218c Mar 22, 2017
@johngruen
Copy link
Contributor Author

@johngruen johngruen commented Mar 22, 2017

@6a68 @Flod, thanks for the review. Sorry about the distracted coding! LMK if there are any more issues here.

@johngruen johngruen force-pushed the provisional-addon-strings branch 4 times, most recently from a7daca7 to 333622c Mar 22, 2017
Copy link
Collaborator

@flodolo flodolo left a comment

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.
Copy link
Collaborator

@flodolo flodolo Mar 22, 2017

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

@johngruen johngruen force-pushed the provisional-addon-strings branch from 333622c to 9577ef4 Mar 22, 2017
@johngruen
Copy link
Contributor Author

@johngruen johngruen commented Mar 22, 2017

Thanks!

@@ -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");
Copy link
Collaborator

@flodolo flodolo Mar 22, 2017

Choose a reason for hiding this comment

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

Funny indentation?

@johngruen johngruen force-pushed the provisional-addon-strings branch from 9577ef4 to e433a21 Mar 22, 2017
@jaredhirsch
Copy link
Member

@jaredhirsch jaredhirsch commented Mar 22, 2017

@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

@jaredhirsch jaredhirsch force-pushed the provisional-addon-strings branch from 7541de2 to 9419640 Mar 22, 2017
@jaredhirsch
Copy link
Member

@jaredhirsch jaredhirsch commented Mar 22, 2017

@johngruen I added the remaining fixes, so you are off the hook here, except for one question: the string tourBodyFour had the awkward phrase Shot Taken Push Notification at the end (see earlier comment). Could you confirm that phrase should have been removed?

We can do another PR if any last-minute changes arrive tomorrow, then I'll bump the bugzilla bug to get localizers started.

<div class="preview-instructions">
<span class="preview-instructions-step-one"></span>
&nbsp;
<span class="preview-instructions-step-two"></span>
Copy link
Member

@jaredhirsch jaredhirsch Mar 22, 2017

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 + &nbsp; + span, and setting the textContent of each span to a different localized string.

Here's how this looks in regular English:

screen shot 2017-03-22 at 1 41 22 pm

If I set dir="rtl" in the body tag on the iframe, English RTL looks like this:

screen shot 2017-03-22 at 1 42 10 pm

Does that look correct?

Finally, the one annoyance about this approach is that, when the screen is narrow, the strings wrap a bit awkwardly:

screen shot 2017-03-22 at 1 41 31 pm

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.

Copy link
Collaborator

@flodolo flodolo Mar 23, 2017

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.

Copy link
Member

@jaredhirsch jaredhirsch Mar 23, 2017

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.

@jaredhirsch
Copy link
Member

@jaredhirsch jaredhirsch commented Mar 22, 2017

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.

@jaredhirsch jaredhirsch force-pushed the provisional-addon-strings branch from 3234cf2 to 461376c Mar 22, 2017
@jaredhirsch jaredhirsch force-pushed the provisional-addon-strings branch from 461376c to 65ab3e2 Mar 23, 2017
@jaredhirsch jaredhirsch merged commit d63c48a into master Mar 23, 2017
1 check passed
@jaredhirsch jaredhirsch deleted the provisional-addon-strings branch Mar 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants