Skip to content
This repository has been archived by the owner on Jul 30, 2022. It is now read-only.

Restructured packages and using different sass-render #15

Closed
wants to merge 3 commits into from
Closed

Restructured packages and using different sass-render #15

wants to merge 3 commits into from

Conversation

chanar
Copy link
Contributor

@chanar chanar commented Apr 3, 2019

Quick overview of changes I have done:

  • No usage of scripts folder, instead using https://www.npmjs.com/package/wc-sass-render
  • Moved package//scss contents to package//src/components/styles
  • Added webpack config from your lit-components folder
  • Demo folder for running components in html
  • Added new commands
"bootstrap": "lerna bootstrap",
"start:watch": "sass-render ./packages/*/src/components/styles/*.scss -w -t sass-template.tmpl",
"start:dev": "webpack-dev-server --open",
"start:storybook": "start-storybook -p 6006",
"start": "npm-run-all --parallel start:*"
  • Removed picka (I was unable to npm run dist as pack was not recognized even after globally installing it. Didn't have any instructions either).

Compilations are now super fast, support running in dev sever and builds.

So first install wc-sass-render globally yarn global add wc-sass-render and then npm run bootstrap

@@ -14,7 +14,7 @@
font-size: 14px;
font-weight: 500;
line-height: 36px;
color: rgb(var(--abm-primary-rgb, 33, 150, 243));
color: RGB(var(--abm-primary-rgb, 33, 150, 243));
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't change the style of the CSS functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right. This is due to node-sass (if i'm not wrong) unable to compile rgb using css variables and default values. sass/node-sass#2251

Copy link
Owner

Choose a reason for hiding this comment

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

The existing script works, and we shouldn't change valid CSS because of weird bugs in tools.

@@ -9,14 +9,16 @@
"build": "yarn build-styling",
"build-styling": "./scripts/build-styling.sh",
"deploy-storybook": "storybook-to-ghpages",
"dist": "lerna run dist",
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't remove this script and relevant @pika/pack sections as they are needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain why you have choosen picka/pack packages. But anyway, was not my intention to remove them. Just to get this stuff running.

Copy link
Owner

Choose a reason for hiding this comment

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

These are needed to prepare ES module version and bundled version for CDN.
Check https://github.com/pikapkg/pack for more details.

And yes, the scss files are in the separate folders intentionally, so as Pack doesn't attempt to process those (it only picks anything inside src).

Copy link
Owner

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

  1. Please remove any .DS_Store files from the PR

  2. Please restore the dist scripts and @pika/pack sections, we need those for publishing.

  3. I don't see any need in committing custom-elements-es5-adapter.js to this repo, neither webcomponents-bundle.js. You should be able to resolve those from the node_modules

@lkraav
Copy link
Contributor

lkraav commented Apr 4, 2019

@web-padawan checked your comments, looks like all achievable. Overall do you see this PR going in a useful direction, though?

@web-padawan
Copy link
Owner

@lkraav yes, the overall direction seems reasonable to me.

The only thing what I would like to keep is folders structure. So basically .scss files should stay in scss folders as they are. When placing them into src, Pack tries to process them and fails.

@lkraav
Copy link
Contributor

lkraav commented Apr 6, 2019

@chanar did you push updates yet? Not seeing anything here.

@chanar
Copy link
Contributor Author

chanar commented Apr 7, 2019

For this, I haven't. I'm still waiting for padawan comment on rgb.

@lkraav
Copy link
Contributor

lkraav commented Apr 7, 2019

@chanar in addition to #15 (comment) or...?

@lkraav
Copy link
Contributor

lkraav commented Apr 7, 2019

@chanar on feature branch development, git pull --rebase is usually the best (read: cleanest, most readable) process vs repeated master merges.

@lkraav
Copy link
Contributor

lkraav commented Apr 7, 2019

@chanar I'm not understanding the need to introduce package-lock.json here, why wouldn't continuing with just yarn not work?

@web-padawan
Copy link
Owner

Yes, please use yarn.lock instead of package-lock.json as the project is using Yarn workspaces.
See also finnhvman/matter#5 (comment) for details about rgb issue in node-sass.

@web-padawan
Copy link
Owner

I merged #18 with adapted changes based on #17 so let me close this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants