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

[Fix] (Workbox wizard) Manually enter the path of the web app root and unselectable separator #2766

Merged
merged 3 commits into from
Mar 10, 2021

Conversation

ognjenjevremovic
Copy link
Contributor

Fixes #2764

R: @jeffposnick @philipwalton

Changes

  • Fixed error thrown during the workbox wizard regarding the web application root directory prompt (first question) when Manually enter path option is chosen,
  • Made the separator unselectable,
  • Extended upon the existing unit tests regarding this functionality.

Checklist

After the changes I ensured that:

  • no stylistic or unwanted errors are present, by running npm run gulp lint from the root directory,
  • no tests are failing, by running npm run gulp test_node from the root directory,
  • build passes with no issues, by running npm run gulp build from the root directory,
  • checked the build output locally by:
    • creating a new npm project and installing the workbox (local) package, by running npm i ~/path/to/local/project/workbox/packages/workbox-cli,
    • spawning the workbox wizard from the project, by running ./node_modules/.bin/workbox wizard,
    • checked the functionality manually with cases covered:
      1. no child directories present,
      2. multiple child directories present,
      3. manual input of existing directory,
      4. manual input of non-existing directory.
    • checking the generated workbox-config.js file content after completing the wizard.

Done

  • Added/updated unit tests for this change
  • Included links to related issues/PRs

Ognjen Jevremovic added 2 commits March 2, 2021 17:55
Make separator unselectable as an answer to root of web application
question when multiple child directories are present. Fix the manual
input to root of the web application question - when manual input option
is chosen a new question / input will be provided to the user to specify
the root of the web app.

✅ Closes: GoogleChrome#2764
Extend upon the existing workbox-cli ask root of web app question with
additional unit tests.

✅ Closes: GoogleChrome#2764
@ognjenjevremovic ognjenjevremovic changed the title Fix/wizard root dir [Fix] (Workbox wizard) Manually enter the path of the web app root and unselectable separator Mar 2, 2021
@jeffposnick jeffposnick self-requested a review March 4, 2021 21:55
@jeffposnick
Copy link
Contributor

Thank you for this! I haven't had time to test the changes locally this week, but I wanted to let you know that it hasn't slipped my mind.

@ognjenjevremovic
Copy link
Contributor Author

No worries. Thank you for getting back to me.
Please do let me know if there are any changes required prior to merging this, once you get time to review the PR.

I'm closely following the workbox project as I familiarized myself with the codebase over the past week(s).
Although, there aren't as many open issues (especially regarding the cli part that I'm most familiar with) I'll be sure to follow the progress and bug reports and try to contribute to the project as much as I can.
Also, I'll try to keep up with the rest of the amazing OSS tools and projects you guys are making freely available for the rest to use and contribute as I see fit.

Thanks and best regards! 🙂

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

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

Thank you! There are just some naming issues to resolve.

workbox-cli package, rename `root of the web application` question
variable names to use camelcase naming pattern instead of snake case
format.

✅ Closes: GoogleChrome#2766
@jeffposnick jeffposnick self-requested a review March 10, 2021 19:00
Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

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

Thanks again! We'll cut a new release that includes these fixes within a week or so.

@jeffposnick jeffposnick merged commit 2c8bfe7 into GoogleChrome:v6 Mar 10, 2021
This was referenced Mar 16, 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.

2 participants