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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI: Add theme param to create #288

Merged
merged 8 commits into from Jan 29, 2020
Merged

CLI: Add theme param to create #288

merged 8 commits into from Jan 29, 2020

Conversation

michalczaplinski
Copy link
Member

@michalczaplinski michalczaplinski commented Jan 24, 2020

Description of what you did:

This PR adds the ability to pick a theme when running the frontity create.

  • A new option --theme which allows specifying the starter theme on the command line.
  • If the theme is not specified, the user can pick a theme from an interactive prompt
  • If the command is run with --no-prompt, fall back to the default theme (mars-theme)

(A few unrelated and small improvements to the tests for the CLI also made it here, by the way)

My PR is a:

  • 馃殌 New feature

Is the PR ready to be merged?

  • 馃毀 Work in progress

Fixes #132

@changeset-bot
Copy link

changeset-bot bot commented Jan 24, 2020

馃 Changeset is good to go

Latest commit: 7e5616d

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

Hey Michal!

There is a problem with the create command. When you select a different theme than mars-theme the frontity.settings.js file won't include the theme you choose but mars. It's the createFrontitySettings function (that is being called here). Could you change that function and add some tests?

Code looks fine, by the way. 馃槉

.changeset/tidy-pets-yawn.md Outdated Show resolved Hide resolved
@luisherranz
Copy link
Member

I've been thinking about this during the last few days and I think we can solve it using the frontity.config.js file: https://community.frontity.org/t/defining-default-settings-for-frontity-packages/1139

Please read that feature topic and let me know what you think (in the topic).

@michalczaplinski
Copy link
Member Author

There is a problem with the create command. When you select a different theme than mars-theme the frontity.settings.js file won't include the theme you choose but mars. It's the createFrontitySettings function (that is being called here).

oh, nice catch @DAreRodz, thanks! 馃檪

Needed in order to populate the settings with the correct package name
@luisherranz
Copy link
Member

Yeah, that is actually the bug!
#132

Sorry we forgot to add it to the sprint. I've done so now and added a Fixes to this PR.

Copy link
Member

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

Looks good to me! 馃憣

@DAreRodz
Copy link
Member

PS: I guess the default settings feature would be implemented in another PR. This is totally fine, isn't it?

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

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

Great work @michalczaplinski!

@luisherranz
Copy link
Member

PS: I guess the default settings feature would be implemented in another PR. This is totally fine, isn't it?

Yeah, I guess this is fine for now.

For reference, this should be the next step: https://community.frontity.org/t/defining-default-settings-for-frontity-packages/1139/3

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