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

feat: Add saber eject-theme command #259

Merged
merged 14 commits into from
Aug 19, 2019

Conversation

krmax44
Copy link
Contributor

@krmax44 krmax44 commented Jun 10, 2019

Summary

Closes #247

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI/CSS related code, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@netlify
Copy link

netlify bot commented Jun 10, 2019

Deploy preview for saber ready!

Built with commit 5eac002

https://deploy-preview-259--saber.netlify.com

@krmax44
Copy link
Contributor Author

krmax44 commented Jun 11, 2019

There's one thing still missing: the file copying works well, but the config does not yet get altered. To add this, the script needs to know where the layout, components etc. files are located, which is not the same for every theme. I came up with a couple of potential solutions:

  • Every theme needs to declare main (or some other custom key) in its package.json, which points to a Saber configuration, related: Support saber config in theme directory #160
  • The script or theme loader should look for the path containing the layouts subdirectory (or a Saber configuration, whatever is a clear sign that this is the theme's root folder). Might be problematic in cases where a dist and source folder both containing a layout folder or other more exotic folder structures.
  • Enforce a strict folder structure every theme needs to use
  • Check for folders like dist and src, partially already present but only for npm themes:

if (/node_modules/.test(this.theme)) {
const distDir = path.join(this.theme, 'dist')
if (fs.existsSync(distDir)) {
this.theme = distDir
}

What do you think? @saberland/core

@nblthree
Copy link
Member

nblthree commented Jun 11, 2019

I prefer using main in package.json.
The reason why I prefer using main is because it's the simplest solution and it's easy to implement for you and for the theme developer. also

The script or theme loader should look for the path containing the layouts subdirectory (or a Saber configuration, whatever is a clear sign that this is the theme's root folder). Might be problematic in cases where a dist and source folder both containing a layout folder or other more exotic folder structures.

This isn't a perfect solution and it can fail in some cases.

Enforce a strict folder structure every theme needs to use

This is annoying for the theme developers.

Check for folders like dist and src, partially already present but only for npm themes

I just don't like this one (Not flexible)

@krmax44
Copy link
Contributor Author

krmax44 commented Jun 13, 2019

@egoist your opinion on this?

@egoist
Copy link
Collaborator

egoist commented Jun 14, 2019

the file copying works well, but the config does not yet get altered

what config?

@krmax44
Copy link
Contributor Author

krmax44 commented Jun 14, 2019

@egoist saber-config.yml

packages/saber/lib/cli.js Outdated Show resolved Hide resolved
packages/saber/lib/cli.js Outdated Show resolved Hide resolved
@egoist
Copy link
Collaborator

egoist commented Jun 21, 2019

@krmax44
Copy link
Contributor Author

krmax44 commented Jun 22, 2019

IIRC, package.json allows specifying something like repository.pathTo, which we could use for this.

@krmax44
Copy link
Contributor Author

krmax44 commented Jul 28, 2019

Added monorepo support and dependency merging. Only thing missing is that saber-config.yml doesn't get updated accordingly, reasons above. Will need to wait for #160

@krmax44 krmax44 requested a review from egoist July 28, 2019 15:21
@krmax44 krmax44 force-pushed the feat/saber-eject-theme branch from ef069e1 to 9f8a8cb Compare August 8, 2019 12:24
@krmax44
Copy link
Contributor Author

krmax44 commented Aug 8, 2019

Okay, I tested it now with portfolio and tailsaw, and it works great so far! What do you think @egoist?

@krmax44 krmax44 requested a review from a team August 13, 2019 08:31
@egoist egoist changed the title feat: Add saber eject command feat: Add saber eject-theme command Aug 19, 2019
@egoist egoist merged commit b45908f into saberland:master Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add theme eject command
3 participants