Conversation
This avoids some fragility if there are overlapping classnames, which will become more likely if we remove pageshot-* prefixes from classes
The save button isn't displayed on the selection screen where this function is applied Also remove pageshot-saver CSS rule, because it now has the class pageshot-highlight-button-save
Also rename .pageshot-myshots-text to .myshots-text, .pageshot-pre|post-myshots to .myshots-text-pre|post
Use .fixed-container for moving-element, to better explain the purpose
Leave historical references in place
…ts' or 'Screenshots'
…ything Use ui.iframe.getElementFromPoint which sets properties properly Change otherwise unused id of the iframe
README.md
Outdated
@@ -12,7 +12,7 @@ It is made up of both an add-on (using [WebExtensions](https://developer.mozilla | |||
|
|||
Ian has been blogging about the [design, definition, and development process](http://www.ianbicking.org/tag/product-journal.html). | |||
|
|||
You can find more information about Page Shot at the Mozilla Wiki page: https://wiki.mozilla.org/Firefox/Page_Shot | |||
You can find more information about Firefox Screenshots at the Mozilla Wiki page: https://wiki.mozilla.org/Firefox/Page_Shot |
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.
@ianb Stale wikimo link, as I'm sure you're aware 👍
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 did not grep for page.shot
so I didn't catch any of these. But there's still a bunch of renaming yet to do, this will go on the list.
# Sets $(PAGESHOT_BACKEND) to http://localhost:10080 only if it isn't set | ||
PAGESHOT_BACKEND ?= http://localhost:10080 | ||
# Sets $(SCREENSHOTS_BACKEND) to http://localhost:10080 only if it isn't set | ||
SCREENSHOTS_BACKEND ?= http://localhost:10080 | ||
|
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.
Also need to update the zip
task ('page_shot', not 'pageshot'), currently fails with
mv: rename web-ext-artifacts/page_shot*.zip to build/pageshot.zip: No such file or directory
docs/METRICS.md
Outdated
|
||
A summary of the metrics Page Shot will record, and what we're looking for in those metrics. | ||
A summary of the metrics Screenshots will record, and what we're looking for in those metrics. |
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 think "Firefox Screenshots" would be clearer than "Screenshots" in some spots in this prose-heavy part (up to the 'usage metrics' section).
@@ -28,7 +28,7 @@ class Body extends React.Component { | |||
<div className="large-icon-message-container"> |
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.
Are you planning to rename the leave-page-shot
directory in a later commit?
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.
another page.shot instance
@@ -312,15 +312,15 @@ class Body extends React.Component { | |||
|
|||
renderExtRequired() { | |||
return <div className="default-color-scheme notification"> | |||
<div> Page Shot is an experimental extension for Firefox. <a href={ this.props.backend } onClick={ this.clickedInstallExtension.bind(this) }>Get it here</a></div> | |||
<a className="close" onClick={ this.closeGetPageshotBanner.bind(this) }></a> | |||
<div> Firefox Screenshots is an experimental extension for Firefox. <a href={ this.props.backend } onClick={ this.clickedInstallExtension.bind(this) }>Get it here</a></div> |
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.
Kinda awkward, but I'm guessing this string ("...is an experimental extension for Firefox") will get updated along with the rest of the new marketing copy?
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 thought about removing "for Firefox", but it seemed potentially confusing given future goals. But yeah, it'll probably get overwritten later.
LGTM so far; the addon and site seem to behave properly. Ping me when it's totally done and I'll do a final pass and merge. |
Document the metrics sent during leaving
Phase 1 of the rename. Primarily focused on CSS classnames that started with
pageshot-