Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Pageshot classname removal #2436

Merged
merged 41 commits into from Mar 21, 2017
Merged

Pageshot classname removal #2436

merged 41 commits into from Mar 21, 2017

Conversation

ianb
Copy link
Contributor

@ianb ianb commented Mar 20, 2017

Phase 1 of the rename. Primarily focused on CSS classnames that started with pageshot-

  • Also removes dead exporting code
  • Remove some vestiges of DOM freezing display
  • Remove lots of styles that didn't apply to anything anymore
  • Rename two metrics to remove "pageshot" from the name

ianb added 30 commits March 20, 2017 14:28
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
@jaredhirsch jaredhirsch self-requested a review March 20, 2017 23:42
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
Copy link
Member

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 👍

Copy link
Contributor Author

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

Copy link
Member

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

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

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?

Copy link
Contributor Author

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

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?

Copy link
Contributor Author

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.

@jaredhirsch
Copy link
Member

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.

@jaredhirsch jaredhirsch merged commit 81212c5 into master Mar 21, 2017
@jaredhirsch jaredhirsch deleted the pageshot-classname-removal branch March 21, 2017 19:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants