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

refactor(bookshop-init): Refactored bookshop init code to save format… #172

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adon-cloudcannon
Copy link

Context

This came about from wanting to use .css files to style bookshop components over .scss. Changing the file extension manually works ok, but having the option when creating a new component would be nice.

That led to adding CSS as an option into bookshop/init - but THAT led to a consideration about whether or not a user would want to specify different formats (yml, toml, etc) per component, or just once across a project... which led to the addition of saving options into the bookshop config file.

THEN... exploring the code, it looked to be 90% set up to prompt the user for all options upon running --new or --component (e.g. project directory, component name, format, style)... so rather than erroring out if the user runs --new or --component without supplied arguments, it prompts them for the required value.

Changes

  • Altered bookshop-config.ejs.t template to save component_directory, format and style along with engines.
  • Added a css.ejs.t template for css files
  • Added an option for CSS to templates.js
  • In main.js
    • Check for format in config file before prompting when creating a new component
    • Check for style in config file before prompting when creating a new component
    • Save new options into config when creating new project
    • Added a bit of info text for when bookshop creates the Sample component on --new (it wasn't clear that it was about to do this, and asked the user to specify their component format etc... so just made it more explicit...)
    • Altered --new and --component functionality to allow user to specify these flags without arguments - bookshop will now prompt for the required info instead of just erroring out
    • Altered --new workflow to prompt user for their format and style choices up front (once per project) - they can choose "I'll decide later" which will then prompt per component. Note: user can at any time manually alter the bookshop.config file and (providing they specify a valid option) they won't be prompted in the future.

Testing (tester)

The reviewer should check on this branch:

  • The code
  • The script locally

Adon Moskal added 2 commits November 8, 2023 11:40
… and style options in config

1. Added option to choose CSS instead of SCSS for bookshop component styles
2. Saved format and style information (and directory) into bookshop config file - use these values if present, otherwise prompt user
3. Added options to --new to specify format and style for the entire project (or decide on a per-component basis)
4. Added a check if the user does --new or --component without specifying a name - shouldn't error out, but rather prompt for value
@bglw
Copy link
Contributor

bglw commented Nov 8, 2023

Nice!

Re: the failing tests, you'll find these files in /javascript-modules/integration-tests/features/init/, and they should be intuitive enough to edit. To run the tests:

  1. Navigate to /javascript-modules/integration-tests/
  2. Run yarn run itest --name "Creating a new Jekyll Bookshop" (where name is the name of a Scenario from any file)
  3. To run all the tests, run yarn run itest-slow in the same directory (as you might expect, it's slow)

First up — if we're prompting people that .css is a valid option for a component, we need the whole stack to understand CSS files as well as SCSS files. Currently if they pick CSS, their framework Bookshop integration won't pick it up.

Tests:

  • The test Compiling simple component Sass could do with a Compiling simple component CSS test added below it, using a css file instead.
  • The Jekyll and Hugo integration test suites don't actually have any SCSS tests — so we should probably add tests for them and then also add CSS tests. I can stub these out for you if you would prefer.

Packages:

@adon-cloudcannon
Copy link
Author

adon-cloudcannon commented Nov 8, 2023

@bglw Totally... agree with all the things...

Re: CSS in Bookshop - I do have a solution that I would love to run by you sometime using postCSS (processes SCSS or CSS or a combination with support for future CSS, etc, etc...) - would be great to get an opinion on it sometime when you have time (which I know is never....)

Re: this PR... would it be worth removing the CSS-specific stuff from the PR for now, but still retaining the 'save options into the config' part? Like the yml format stuff? Because that's useful on its own maybe?

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.

2 participants