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

Support config file for arguments #9

Merged
merged 58 commits into from
Jan 21, 2021
Merged

Conversation

laras126
Copy link
Collaborator

@laras126 laras126 commented Oct 15, 2020

This PR contains two main additions:

  • Adds the ability to store audit arguments in a configuration file named css-audit.config.js in the root of the project (a follow up PR can contain smarter config storage using cosmiconfig, for example) - fixes Use a config file to configure audit options #8
  • Supports multiple property-values audits from the config

A few notes:

  1. The configuration format is slightly different than what is outlined in Use a config file to configure audit options #8 to work more smoothly with the format that comes from the CLI, but this can be updated if required.
  2. Multiple property-values audits are only supported from configuration, I did not add support for the CLI (though possible if we need this)

A couple other changes to note:

  • Refactor the index.js and audit running into a function so that it can be tested
  • Remove destructuring of results from the JSON format and output the whole result object (this was easier to test and has no other impact as far as I can tell)
  • Support for formatting nested directories in the prettier globbing

To test:

Pull down this branch and update the css-audit.config.js to experiment with different audit configurations. Run the command npm run css-audit -- path/to/css and confirm that the results display according to what is in the config.

Make sure the CLI overrides options added in the config e.g. npm run css-audit -- path/to/css --format=cli-table. Should display the results as a CLI table.

Still to do:

  • Update README
  • Add audits we need from the Google doc
  • Use cosmiconfig for smarter config file locating

@laras126 laras126 marked this pull request as draft November 12, 2020 21:56
@laras126
Copy link
Collaborator Author

laras126 commented Nov 12, 2020

I have a couple of things left to do, but @ryelle I think this is ready for a first look!

Remaining items (might add to this list):

  • Don't use the config file if a --config is not present (right now this logic is based on the presence of the css-audit.config.js file and it's getting in the way)
  • Make sure all of the audits work - most do
  • Make sure the html format works and the template gets the property value data
  • Rename one of the run.js files

Bonus feature:

  • Ability to show categories for the property-values in the HTML version (maybe have a "title" key in the config object or something)

@ryelle
Copy link
Collaborator

ryelle commented Nov 16, 2020

I pulled some of the fixes here out into master:

  • c706bb9: ESLint: Allow console commands, remove inline exceptions
  • 896de68: Audits: Add audit key to media queries & property values
  • dc1cdd6: Remove unused CSS file

Instead of creating runAuditsFromConfig & runAuditsFromCLIArgs, what about creating getArg/getArgs overlays, which get the property from the CLI, and fall back to the config file if it exists? Right now, the filename for reports isn't working since it still uses getArgFromCLI, but it could just use getArg to get the filename wherever it's defined.

@laras126 laras126 changed the title WIP: Config file for options Support Config file for Arguments Dec 10, 2020
@laras126 laras126 changed the title Support Config file for Arguments Support config file for arguments & support multiple property-value audits Dec 10, 2020
@laras126 laras126 changed the title Support config file for arguments & support multiple property-value audits Support config file for arguments Dec 10, 2020
@laras126 laras126 marked this pull request as ready for review January 6, 2021 22:21
@laras126
Copy link
Collaborator Author

laras126 commented Jan 6, 2021

@ryelle This is ready to go in! There are a few things remaining that I think will be best in follow up PRs given how long this one has been open:

  • Support multiple audit configurations e.g. wp-admin and other-project
  • Option in the CLI to bypass the configuration file - right now if no corresponding arg exists in the CLI, whatever is in the configuration file will be respected. If that is a big set of audits, it is cumbersome to only run a single audit.
  • Support "short names" for property-value audits - see screenshot below.

Screen Shot 2021-01-06 at 5 27 13 PM

Copy link
Collaborator

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

🎉 It's working great! Tested all 3 formats (cli, json, and HTML), and with a few different configuration tweaks. Ran into just one issue, and noted a few other minor things, most coming from the changes in master.

Will the values used in the config file for property-values be changed before merge? It looks like test cases, but I just want to check 🙂

README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/formats/templates/audits/property-values.twig Outdated Show resolved Hide resolved
src/formats/html.js Outdated Show resolved Hide resolved
src/run.js Outdated Show resolved Hide resolved
@laras126
Copy link
Collaborator Author

laras126 commented Jan 13, 2021

@ryelle Thanks for the review - feedback addressed! I removed the excessive property-values from the audit until we decide how we will work them in.

Will the values used in the config file for property-values be changed before merge? It looks like test cases, but I just want to check 🙂

I had copied what was in the CSS audit Google Doc - these are what we will want to see eventually but as we have seen, there's some additional thinking to do! I updated it to include just one property value for font-size.

@laras126 laras126 requested a review from ryelle January 13, 2021 21:11
@danfarrow
Copy link
Collaborator

danfarrow commented Jan 14, 2021

I noticed an additional effect of the Property Values navigation issue: Because those report sections all have id="property-values" all of the Property Values nav links just jump to the first section. This could also be fixed once we have a short-names fix

Copy link
Collaborator

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

🎉 This is working great! I left one comment about the config file itself, but we can merge this and iterate on the config (since there are also questions about property-values).

Thanks for all your hard work here!

Comment on lines +3 to +5
all: true,
filename: 'wp-admin',
audits: [ [ 'property-values', 'font-size' ] ],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd rather leave the all property out of the config file - it was intended as a shortcut for the CLI command, but in a config file I think it would be better to list out each audit intentionally.

This doesn't need any special handling, I think we can just remove it from this file.

# Conflicts:
#	public/wp-admin.html
@ryelle ryelle merged commit 0df68c9 into WordPress:master Jan 21, 2021
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.

Use a config file to configure audit options
3 participants