-
Notifications
You must be signed in to change notification settings - Fork 69
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
[BD-46] feat: move CLI design tokens commands to paragon CLI #2609
[BD-46] feat: move CLI design tokens commands to paragon CLI #2609
Conversation
Thanks for the pull request, @monteri! When this pull request is ready, tag your edX technical lead. |
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
I propose to add descriptions for newly added commands after the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #2609 +/- ##
==========================================
+ Coverage 92.09% 93.28% +1.19%
==========================================
Files 215 215
Lines 3616 3635 +19
Branches 890 898 +8
==========================================
+ Hits 3330 3391 +61
+ Misses 281 239 -42
Partials 5 5 ☔ View full report in Codecov by Sentry. |
7bb10d2
to
ff2569c
Compare
dfd8156
to
b363867
Compare
b363867
to
e9eedf9
Compare
140bb96
to
1de83ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PKulkoRaccoonGang Left some initial comments, I will try to take another look later today
bin/paragon-scripts.js
Outdated
if (command === HELP_COMMAND) { | ||
helpCommand(COMMANDS); | ||
helpCommand(COMMANDS, commandArgs); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this code if you define help's executor as follows:
help: {
executor: (args) => helpCommand(COMMANDS, args),
...
then try / catch below will be able to correctly execute help command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
lib/build-scss.js
Outdated
ora().succeed(chalk.underline.bold.green(`Stylesheets for ${capitalize(name)} build successfully!`)); | ||
// eslint-disable-next-line no-console | ||
console.log(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can avoid using console.log
by adding \n
to the previous line ora().succeed(chalk.underline.bold.green("Stylesheets for ${capitalize(name)} build successfully!\n"));
Also, right now it says the following when I run the command
I think it would be better to change the wording to something like this
just a suggestion (although I think we definitely need to add word theme
somewhere, just saying Light
/ Core
is not enough I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it looks better
lib/build-scss.js
Outdated
function buildScssCommand(commandArgs) { | ||
const argv = minimist(commandArgs); | ||
const corePath = argv.corePath || path.resolve(process.cwd(), 'styles/scss/core/core.scss'); | ||
const themesPath = argv.themesPath || path.resolve(process.cwd(), 'styles/css/themes'); | ||
const outDir = argv.outDir || './dist'; | ||
const defaultThemeVariants = argv.defaultThemeVariants ? argv.defaultThemeVariants.split(',') : ['light']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading through minimist docs I found that you can provide default values to args, I think we should try doing that, right now it looks a little dirty
this seems to work for me locally (also, I don't think you need to split defaultThemeVariants
)
function buildScssCommand(commandArgs) {
const defaultArgs = {
corePath: path.resolve(process.cwd(), 'styles/scss/core/core.scss'),
themesPath: path.resolve(process.cwd(), 'styles/css/themes'),
outDir: './dist',
defaultThemeVariants: 'light',
};
const {
corePath,
themesPath,
outDir,
defaultThemeVariants,
} = minimist(commandArgs, { default: defaultArgs });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
37ee123
to
db966ac
Compare
README.md
Outdated
- `paragon install-theme [theme]`: Installs the specific [@edx/brand](https://github.com/edx/brand-edx.org) package. | ||
- `build-tokens`: Build Paragon's design tokens. | ||
- `replace-variables`: Replace SCSS variables usages or definitions to CSS variables and vice versa in `.scss` files. | ||
- `build-scss`: Compile Paragon's core and themes SCSS into CSS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we prefer paragon command-name
or just command-name
for the README? I'm happy either way, but I think it should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it makes sense to be consistent, updated it to paragon command-name
format
README.md
Outdated
@@ -61,7 +61,10 @@ The Paragon CLI (Command Line Interface) is a tool that provides various utility | |||
|
|||
### Available Commands | |||
|
|||
- `paragon install-theme [theme]`: Installs the specific @edx/brand package. | |||
- `paragon install-theme [theme]`: Installs the specific [@edx/brand](https://github.com/edx/brand-edx.org) package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should reference the openedx brand package instead of the edx.org one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, updated, thanks.
@monteri 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 22.0.0-alpha.12 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 23.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* feat: move cli design tokens to paragon cli * feat: update paths for build-tokens.js * feat: added descriptions for CLI commands * refactor: removed commander implementation * refactor: added build-scss process status * feat: added ora compilation status * refactor: code refactoring * feat: added help description for single command * refactor: after review * refactor: refactoring after review * chore: update docs and cli params parsing --------- Co-authored-by: monteri <lansevermore> Co-authored-by: PKulkoRaccoonGang <[email protected]> Co-authored-by: Viktor Rusakov <[email protected]>
* feat: move cli design tokens to paragon cli * feat: update paths for build-tokens.js * feat: added descriptions for CLI commands * refactor: removed commander implementation * refactor: added build-scss process status * feat: added ora compilation status * refactor: code refactoring * feat: added help description for single command * refactor: after review * refactor: refactoring after review * chore: update docs and cli params parsing --------- Co-authored-by: monteri <lansevermore> Co-authored-by: PKulkoRaccoonGang <[email protected]> Co-authored-by: Viktor Rusakov <[email protected]>
* feat: move cli design tokens to paragon cli * feat: update paths for build-tokens.js * feat: added descriptions for CLI commands * refactor: removed commander implementation * refactor: added build-scss process status * feat: added ora compilation status * refactor: code refactoring * feat: added help description for single command * refactor: after review * refactor: refactoring after review * chore: update docs and cli params parsing --------- Co-authored-by: monteri <lansevermore> Co-authored-by: PKulkoRaccoonGang <[email protected]> Co-authored-by: Viktor Rusakov <[email protected]>
* feat: move cli design tokens to paragon cli * feat: update paths for build-tokens.js * feat: added descriptions for CLI commands * refactor: removed commander implementation * refactor: added build-scss process status * feat: added ora compilation status * refactor: code refactoring * feat: added help description for single command * refactor: after review * refactor: refactoring after review * chore: update docs and cli params parsing --------- Co-authored-by: monteri <lansevermore> Co-authored-by: PKulkoRaccoonGang <[email protected]> Co-authored-by: Viktor Rusakov <[email protected]>
🎉 This PR is included in version 23.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Include a description of your changes here, along with a link to any relevant Jira tickets and/or Github issues.
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist