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

Move no-old-flexbox to table formatter #1545

Merged
merged 7 commits into from
Jan 31, 2017
Merged

Move no-old-flexbox to table formatter #1545

merged 7 commits into from
Jan 31, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Jan 26, 2017

R: @patrickhulce all

This PR:

  • add <pre> support to the markdown renderer
  • fixes a bug in the no-old-flexbox audit where it was not finding all old flexbox uses. The audit was incorrectly testing vendor prefixed css properties and matching them with prefixed values. They need to be run separately.
  • adds some smokehouse checks to cache all the pokemon
  • moves the audit to use the table formatter.
  • attributes inline styles to "inline". Issue in CSS flexbox audit no longer contains urls #1500.

screen shot 2017-01-26 at 11 31 11 am

@ebidel ebidel changed the title Add TABLE formatter to NoFlexBox audit Move no-old-flexbox to table formatter Jan 26, 2017
@brendankenny
Copy link
Member

to cache all the pokemon

so close :P

sheet.parsedContent.forEach(props => {
const formattedStyleRule = StyleHelpers.getFormattedStyleRule(sheet.content, props);

let url = sheet.header.sourceURL;
if (!url) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also check if it matches the pageUrl if we want to be consistent. Also thoughts on using getDisplayName here?

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

@patrickhulce
Copy link
Collaborator

I have a fix in #1536 for the wonky indentation of the I can split out too btw

@ebidel
Copy link
Contributor Author

ebidel commented Jan 26, 2017

caching pokemon is important for great performance

@@ -18,3 +18,6 @@
body {
background-color: #eee;
}
.doesnotapply {
display: flexbox; /* FAIL */
}
Copy link
Member

Choose a reason for hiding this comment

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

new line

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

urlList.push({
url: sheet.header.sourceURL,
label: formattedStyleRule.location,
url,
Copy link
Member

Choose a reason for hiding this comment

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

url: sheet.header.sourceURL || 'inline', seems a bit clearer here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in the url === pageUrl check above

@@ -96,14 +96,14 @@ function getFormattedStyleRule(content, parsedContent) {
}, []).join('\n');
}

const block = `
${parsedContent.selector} {
const block = `\`\`\`${parsedContent.selector} {
Copy link
Member

Choose a reason for hiding this comment

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

doing this in the extendedInfo value seems a bit ugly. What about a different keyword for the table formatter that annotates code blocks with the ```s?

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


return {
styleRule: block.trim(),
location: `line: ${startLine}, row: ${start}, col: ${end}`
startLine,
location: `${start}:${end}`
Copy link
Member

Choose a reason for hiding this comment

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

we should think about settling on a format for parsed source blocks at some point. ESTree has a semi-standard format (albeit for JS): https://github.com/estree/estree/blob/master/es5.md#node-objects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@ebidel
Copy link
Contributor Author

ebidel commented Jan 26, 2017

PTAL

@brendankenny
Copy link
Member

needs a rebase due to table churn

@ebidel
Copy link
Contributor Author

ebidel commented Jan 28, 2017

rebased!

@@ -86,6 +86,8 @@ class Table extends Formatter {
switch (key) {
case 'code':
return '`' + value.trim() + '`';
case 'pre':
return '\`\`\`\n' + result[key].trim() + '\`\`\`';
Copy link
Member

Choose a reason for hiding this comment

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

add this to table-formatter-test.js?

Copy link
Member

Choose a reason for hiding this comment

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

oh, and added to the jsdoc above :)

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

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

💪
┳┳

@brendankenny brendankenny merged commit 003f7b4 into master Jan 31, 2017
@brendankenny brendankenny deleted the moretables branch January 31, 2017 01:57
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

3 participants