Skip to content
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

Extension backport #49

Merged
merged 15 commits into from Mar 22, 2016
Merged

Extension backport #49

merged 15 commits into from Mar 22, 2016

Conversation

paullewis
Copy link
Contributor

Builds on @brendankenny's patch; does a total refactor of many things.

But now.... 🌟 The extension and node versions share the same audits! 🌟

🎉

var flattenedAudits = results.reduce(function(prev, curr) {
class Auditor {

flattenArtifacts_(artifacts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets _ prefix this just to mark it as "private"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://google.github.io/styleguide/javascriptguide.xml#Visibility__private_and_protected_fields_ indicates that private and protected are suffixed not prefixed :-/

Copy link
Contributor

Choose a reason for hiding this comment

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

let's do something at least

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

const manifest = manifestParser(inputs.manifest).value;

if (manifest && manifest.icons) {
const icons192 = manifest.icons.raw.find(function(i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit i => e.sizes === '192x192'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"manifest_version": 2,
"description": "__MSG_appDescription__",
Copy link
Contributor

Choose a reason for hiding this comment

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

this was for internationalization which we might want to continue using :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not know. Will fix.

paullewis added a commit that referenced this pull request Mar 22, 2016
@paullewis paullewis merged commit 5a8849d into GoogleChrome:master Mar 22, 2016
@paullewis paullewis deleted the extension-backport branch March 22, 2016 16:27
@paullewis
Copy link
Contributor Author

Closes #48 - @brendankenny I folded you work into what I was doing, as discussed.

var flattenedAudits = results.reduce(function(prev, curr) {
class Auditor {

_flattenArtifacts(artifacts) {
Copy link
Member

Choose a reason for hiding this comment

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

these should be static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

ha, I didn't mean to state that so strongly. More like they can be static since they aren't keeping track of state or anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fine. I changed it.

const manifest = manifestParser(inputs.manifest).value;

if (manifest) {
hasName = (!!manifest.name);
Copy link
Member

Choose a reason for hiding this comment

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

!!manifest.name.value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants