liferay / generator-liferay-fragments

Yeoman generator for creating and maintaining Liferay Fragment projects
https://www.npmjs.com/package/generator-liferay-fragments
MIT License
30 stars 31 forks source link

test: Add SCSS sample project (LPS-146880) #221

Closed p2kmgcl closed 2 years ago

p2kmgcl commented 2 years ago

This pull request is an initial draft based on https://github.com/liferay/generator-liferay-fragments/pull/152 proposal. The goal is to compile SCSS files before they are being zipped or sent to Liferay Portal, and provide the built CSS code.

In your fragment.json file, now you can use a .scss or .sass file to trigger the SCSS build, and a *.css will be generated instead. All other .scss files will be deleted in the final result. The fragment.json file will also be updated to point to the new CSS file. So this:

my-fragment/
  fragment.json (content: { cssPath: "index.scss })
  index.html
  index.js
  index.scss
  dependency-a.scss
  lib/
    _some-other.sass

Will be transformed into this:

my-fragment/
  fragment.json  (content: { cssPath: "index.css })
  index.html
  index.js
  index.css

:warning: As we are doing with React fragments, only compiled code will be stored in portal, so if you try to export the generated fragment, the SCSS code will not be available. As an improvement, we might want to disallow exporting SCSS fragments to prevent this.

d80wrk commented 2 years ago

This will really add a LOT of value to the developer experience. No longer would I need to use an external compiler to get my CSS into a fragment. It really slows down fragment development time to not be able to use SCSS directly in a fragment.

p2kmgcl commented 2 years ago

This will really add a LOT of value to the developer experience. No longer would I need to use an external compiler to get my CSS into a fragment. It really slows down fragment development time to not be able to use SCSS directly in a fragment.

Great to now :smile:

If you can think of something that I am missing in this implementation or you have the opportunity to test it please do. I will have a look at the windows issues that are appearing on the tests tomorrow.

faragos commented 2 years ago

Hey 😄 Nice to hear from you. I don't have time to check it in depth, but it looks not bad. The thing I'm missing from my implementation is the includePaths. In Dart Sass it should be still the same. (See comment here https://github.com/sass/dart-sass/issues/1058#issuecomment-675086079) This property would be useful to share the different SASS variables from the theme, which will remain even with custom properties. (For example the breakpoints)

p2kmgcl commented 2 years ago

Hey smile Nice to hear from you. I don't have time to check it in depth, but it looks not bad. The thing I'm missing from my implementation is the includePaths. In Dart Sass it should be still the same. (See comment here sass/dart-sass#1058 (comment)) This property would be useful to share the different SASS variables from the theme, which will remain even with custom properties. (For example the breakpoints)

True, I forgot about the includePaths option, maybe we can include it as an optional key in fragment.json, something like "sass": { "includePaths": [] } like you specified in the previous PR.

p2kmgcl commented 2 years ago

Currently I am stuck testing it on windows, I saw this errors both in CI and my console:

    Failed: JsNoSuchMethodError {
      "$thrownJsError": [TypeError: J.getInterceptor$ax(...).map$1$1 is not a function],
      "__js_helper$_message": "J.getInterceptor$ax(...).map$1$1 is not a function",
      "_method": "map$1$1",
      "_receiver": undefined,
    }

but because SASS is written in Dart debugging that code is a huge headache :sweat:.

image

it looks like there is something wrong with the path being passed to SASS, but I double checked and that path is absolute and normalized (something like C:\Users\User\AppData\Local\Temp\generator-liferay-fragments-12-690774061910006-1644318255069\src\sample-collection\sample-fragment\styles.scss) so I don't know what is going on.

Maybe we should be passing an UNIX path here, but I haven't tried :shrug:

faragos commented 2 years ago

Maybe we should be passing an UNIX path here, but I haven't tried 🤷

Ye I would try to use "/" instead of "\" to be sure it's because of that. But I'm not sure if you have control over that path, because I think this path is the path to the file and you get it through glob? Sadly, I can't provide more info. But yes Dart looks annoying to debug 🙈

p2kmgcl commented 2 years ago

That path is built like this:

path.resolve(
  builtProjectContent.basePath,
  'src',
  collection.slug,
  fragment.slug,
  fragment.metadata.cssPath
)

I tried to do a fs.existsSync and the file is there, so it has to be something to to with SASS. Also tried:

But nothing works.

I'm leaving it until tomorrow and see if I can come up with something else.

d80wrk commented 2 years ago

I'd be happy to test on my environment if I can get it going.

Have you guys also seen http://sass.js.org ? It would be very useful to be able to use SASS in the Liferay DXP Web Portal UI, the fragment editor? Even if it's a case that I would paste my SCSS in and then compile in the browser to save as CSS to the server for the fragment that would be a huge step up in capability in Liferay.

p2kmgcl commented 2 years ago

Have you guys also seen http://sass.js.org ? It would be very useful to be able to use SASS in the Liferay DXP Web Portal UI, the fragment editor? Even if it's a case that I would paste my SCSS in and then compile in the browser to save as CSS to the server for the fragment that would be a huge step up in capability in Liferay.

We have no plans to do that

p2kmgcl commented 2 years ago

Ok I have tried to use "sass" binary instead of calling the API and it looks like it's working. I don't know what was happening with the file path, but it looks like sending it's as CLI parameter works :shrug:

@faragos I have also included "sass: loadPaths" in fragment.json :partying_face:, can you please have a look?

I think the PR should be ready.

d80wrk commented 2 years ago

Have you guys also seen http://sass.js.org ? It would be very useful to be able to use SASS in the Liferay DXP Web Portal UI, the fragment editor? Even if it's a case that I would paste my SCSS in and then compile in the browser to save as CSS to the server for the fragment that would be a huge step up in capability in Liferay.

We have no plans to do that

Is it possible to make plans to do that?

p2kmgcl commented 2 years ago

Have you guys also seen http://sass.js.org ? It would be very useful to be able to use SASS in the Liferay DXP Web Portal UI, the fragment editor? Even if it's a case that I would paste my SCSS in and then compile in the browser to save as CSS to the server for the fragment that would be a huge step up in capability in Liferay.

We have no plans to do that

Is it possible to make plans to do that?

Following the regular workflow: opening a feature request en JIRA, then passing PM validation, etc. :smile:

faragos commented 2 years ago

@faragos I have also included "sass: loadPaths" in fragment.json 🥳, can you please have a look?

Looks good 👍 Even tho we will use the config, maybe there are people that don't use this feature. I'm not sure if it still works with sass: true but I didn't see a test for it but I would recommend it☺️ I'm exited for it🥳

p2kmgcl commented 2 years ago

@faragos I have also included "sass: loadPaths" in fragment.json partying_face, can you please have a look?

Looks good +1 Even tho we will use the config, maybe there are people that don't use this feature. I'm not sure if it still works with sass: true but I didn't see a test for it but I would recommend itrelaxed I'm exited for itpartying_face

Ah you can omit the whole "sass" configuration in fragment.json and it should work, everything is optional. You only need to specify a .sass or .scss file in the "cssPath" field and it will use SASS compiler.

I am going to add another test with that example.

p2kmgcl commented 2 years ago

@victorg1991 I have published 2.0.0-rc1 so you can test it :smile:

p2kmgcl commented 2 years ago

Thanks @victorg1991 :smile:

@faragos I have published a pre-release, do you think you'll have some time to test it? You only need to install generator-liferay-fragments@2.0.0-rc1

faragos commented 2 years ago

@faragos I have published a pre-release, do you think you'll have some time to test it? You only need to install generator-liferay-fragments@2.0.0-rc1

Nice. I can't test it great atm, because I have covid🤕. I can maybe test it on Monday?

p2kmgcl commented 2 years ago

Nice. I can't test it great atm, because I have covidface_with_head_bandage. I can maybe test it on Monday?

:scream: sure you can. Hope you feel better this week :candle:

JannikJost commented 2 years ago

Hey, @faragos asks me to test this for him. For me, it looks like it doesn't work yet. I downloaded the version “2.0.0-rc1” and tried the following: { "cssPath": "styles.scss", "configurationPath": "configuration.json", "htmlPath": "index.html", "jsPath": "main.js", "name": "test", "type": "component" }

I also installed SASS: grafik

But if I try to import the fragment, the following error pops up:

06417855-1644846021667\src\action\test-1\styles.scss C:\Users\jjo\AppData\Local\Temp\generator-liferay-fragments-0-8541788206417855-1644846021667\src\action\test-1\styles.css --load-path=C:\Users\jjo\AppData\Local\Temp\generator-lif
eray-fragments-0-8541788206417855-1644846021667\src\action\test-1
The command "C:\dev\...\fragments\node_modules\generator-liferay-fragments\node_modules\.bin\sass" is either misspelled or could not be found.

What am I doing wrong? Or is it still broken?

p2kmgcl commented 2 years ago

@JaJost but did you installed the toolkit globally? It needs to be installed as a project dependency.

faragos commented 2 years ago

Hey @p2kmgcl It's me again and mostly fit again😄 @JaJost Thank you for testing it in the meanwhile. Sadly it didn't work for you but we can have a look together.

I tried it on my PC and it worked nicely 🥳 I have 1 possible improvement and 1 problem with the solution:

Improvement I would remove the source-map generation, because you don't serve it and it will lead to a warning in the console of the browser. I think you can simply pass the --no-source-map flag.

Problem Maybe it's specific but also the main purpose of my loadPaths-implementation. I try to explain it why we need it and what's the benefit. And also why it's doesn't work ATM with the implementation. You are free to say: "Ye, we don't do that", but I think it's important for Liferay to see the whole picture 😃 Every project with custom styling has and will have a theme right? In this Theme every customer will define their variables. Most of them will be custom properties in the future and not SASS Variables. But there are SASS Variables that will stay. For example the breakpoints. In Fragments you want to use these breakpoints. So far we did this by: @import "../../../../../themes/clavis-theme/src/css/clavis/variables"; With the loadPaths I tried to achieve, that the import will be this: @import "variables"; The implementation allows at the moment only absolute paths (which makes kinda sense because you copy the module to a temp folder I saw) Absolute paths have the problem, that I can't share them as easily with my colleagues over Git. I think my implementation didn't copy the files to a temp folder, therefore relative paths were possible.

If you have a possible solution for this problem I'm open to it. Otherwise I'm a big fan of the solution and I don't want to prolong this PR beside the mentioned improvement.

Good work and I'm looking forward to use it 👍

p2kmgcl commented 2 years ago

I would remove the source-map generation, because you don't serve it and it will lead to a warning in the console of the browser. I think you can simply pass the --no-source-map flag.

You are right, I am removing source map generation :smile:

p2kmgcl commented 2 years ago

Maybe it's specific but also the main purpose of my loadPaths-implementation. I try to explain it why we need it and what's the benefit. And also why it's doesn't work ATM with the implementation. You are free to say: "Ye, we don't do that", but I think it's important for Liferay to see the whole picture Every project with custom styling has and will have a theme right? In this Theme every customer will define their variables. Most of them will be custom properties in the future and not SASS Variables. But there are SASS Variables that will stay. For example the breakpoints. In Fragments you want to use these breakpoints. So far we did this by: @import "../../../../../themes/clavis-theme/src/css/clavis/variables"; With the loadPaths I tried to achieve, that the import will be this: @import "variables"; The implementation allows at the moment only absolute paths (which makes kinda sense because you copy the module to a temp folder I saw) Absolute paths have the problem, that I can't share them as easily with my colleagues over Git. I think my implementation didn't copy the files to a temp folder, therefore relative paths were possible.

Speaking about this, let me check because I think you do can use relative paths in the configuration. I am giving it a try.

p2kmgcl commented 2 years ago

Ok you are right, but I agree that it should also resolve relative paths, so I am trying to fix that too :smile:

Thanks so much for testing!

p2kmgcl commented 2 years ago

I have published v2.0.0-rc2 with these changes. I am merging these changes for now so I can continue working on the story. I'll try to release a stable 2.0.0 soon when I have time to cleanup the repo, but feel free to use the pre-release until then :smile:

p2kmgcl commented 2 years ago

https://github.com/liferay/generator-liferay-fragments/releases/tag/v2.0.0-rc2

jwanner83 commented 2 years ago

I just migrated about 30 fragments to version 2.0.0 of the generator-liferay-fragments and was able to remove our own custom CSS preprocessing process. The new functionality works great and is really useful 💯 Thank you guys!