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: added sass support and removed less support #2348

Merged
merged 4 commits into from
Jun 5, 2024
Merged

feat: added sass support and removed less support #2348

merged 4 commits into from
Jun 5, 2024

Conversation

agliga
Copy link
Contributor

@agliga agliga commented Jun 3, 2024

Creating a draft initially. Still have some minor things to work through

  • Swapped all less files to scss. I left the old less mixins (and will probably leave the variables one) to give teams a chance to migrate away from it
  • I did not change the root directory or paths. I will do that if we are happy with this approach.
  • There is also misc cleanup needed to be done.

Questions to be answered

  • I did not fix the docs auto-reload. CSS auto reloads but html does not. The reason is if we are to swap to marko run, that would be throw away work. If this is necessary until we swap to marko run, I check if it works or not.
  • We are using postcss on the processed compiled scss. Basically we compile the SCSS code to CSS and then run postcss with autoprefixer and nano on it. If there is anything else to run on this, let me know.
  • Im planning on running the tests before we release.

If the first pass looks good, I will go ahead and rework the paths and do other cleanups, like remove gulpfile, dev deps no longer used etc.

Copy link

changeset-bot bot commented Jun 3, 2024

🦋 Changeset detected

Latest commit: 6765dec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ebay/skin Patch

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
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looks great! Just a couple of things...

dist/alert-dialog/alert-dialog.css Outdated Show resolved Hide resolved
--dialog-lightbox-max-width: 616px;
--dialog-scrim-color-hide: #11182000;
--dialog-scrim-color-show: #111820b3;
--dialog-lightbox-max-width: 616px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the change in indentation intentional? Unless there was an oversight previously, it would be best to keep the same indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe prettier was not run on the app. I simply ran prettier which changed the indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, for more context
Dist was in the prettier ignore list. I believe this was due to it changing the format and this might have been picked up on npm run build
I changed it so npm run build runs prettier on dist folder. This is to keep the same formatting across the app.

docs/_includes/less.html Show resolved Hide resolved
@agliga agliga marked this pull request as ready for review June 5, 2024 13:48
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looks great! Nice work!

@agliga agliga merged commit 86621e8 into 18.0.0 Jun 5, 2024
2 checks passed
@agliga agliga deleted the postcss branch June 5, 2024 23:13
@github-actions github-actions bot mentioned this pull request Jun 5, 2024
@agliga agliga mentioned this pull request Jun 5, 2024
6 tasks
@github-actions github-actions bot mentioned this pull request Jun 28, 2024
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