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.

Sign up for GitHub

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

Jump to bottom

[IMPL] - added support for sass and scss stylesheet languages #4292

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

[IMPL] - added support for sass and scss stylesheet languages #4292

wants to merge 28 commits into from
+155 −12

Conversation

KronosDev-Pro
Copy link

  • better checking of stylesheets to be compiled
  • added support for sass and scss stylesheet languages
  • the stylesheets files are now copied to ".web/styles/" at compile time
  • relock poetry file for libsass deps
  • stylesheet compiler unit tests also check the contents of the file

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@KronosDev-Pro
[IMPL] - added support for sass and scss stylesheet languages
0681cc2 
 - better checking of stylesheets to be compiled
 - added support for sass and scss stylesheet languages
 - the stylesheets files are now copied to ".web/styles/" at compile time
 - relock poetry file for libsass deps
 - stylesheet compiler unit tests also check the contents of the file
picklelo
picklelo reviewed Nov 4, 2024
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
adhami3310
adhami3310 reviewed Nov 7, 2024
View reviewed changes
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
adhami3310
adhami3310 requested changes Nov 7, 2024
View reviewed changes
Copy link
Member

@adhami3310 adhami3310 left a comment

Choose a reason for hiding this comment

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

This looks much better now, a few nitpicks and we should be good to go !

reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
@KronosDev-Pro
Copy link
Author

Sorry, that's why I can't see the error...

Unit tests, cp312
Unit tests, cp310

@KronosDev-Pro
Copy link
Author

KronosDev-Pro commented Nov 11, 2024 •
edited
Loading

Units tests

python package check
cp39
+libsass ✅
-libsass ✅
cp310
+libsass ✅
-libsass ✅
cp311
+libsass ✅
-libsass ✅
cp312
+libsass ✅
-libsass ✅

@KronosDev-Pro
Copy link
Author

well, for the integration-node-latest / check_latest_node (3.12, 1, node), I don't know why he raises some errors

adhami3310
adhami3310 reviewed Nov 12, 2024
View reviewed changes
reflex/compiler/compiler.py Outdated Show resolved Hide resolved
tests/units/compiler/test_compiler.py
@@ -165,7 +239,7 @@ def test_compile_stylesheets_exclude_tailwind(tmp_path, mocker):

assert compiler.compile_root_stylesheet(stylesheets) == (
str(Path(".web") / "styles" / "styles.css"),
"@import url('../public/styles.css'); \n",
"@import url('./styles.css'); \n",
Copy link
Member

Choose a reason for hiding this comment

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

are you sure this won't break anything?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know where this could cause a problem, since all the supported stylesheet files in the app/assets directories and subdirectories are copied or compiled and saved in the .web/styles directory and no longer in the .web/public directories.

and also, I checked the second batch of the tests/integration group and the tests/integration/test_tailwind.py (which is the only one to have tests impacted by the changes) passed the tests without any errors, which is why I don't understand the error in the first batch of the tests/integration group.

@KronosDev-Pro
remove unused var & import
4fae418
@KronosDev-Pro
[IMPL] - added support for sass and scss stylesheet languages
613e130 
 - better checking of stylesheets to be compiled
 - added support for sass and scss stylesheet languages
 - the stylesheets files are now copied to ".web/styles/" at compile time
 - relock poetry file for libsass deps
 - stylesheet compiler unit tests also check the contents of the file
@KronosDev-Pro
fix f-string bug
81dab90
@KronosDev-Pro
make "libsass" an optional dependency
e815903
@KronosDev-Pro
remove libsass of deps list
6e32fcb
@KronosDev-Pro
fix test caused by optional "libsass" deps
2ca363c
@KronosDev-Pro
improving _compile_root_stylesheet function and add folder styleshe…
6898a26 
…ets unit test
@KronosDev-Pro
fix the copy files in assets to public folder
59a0cbf
@KronosDev-Pro
remove useless f-string
6a2a5ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adhami3310 adhami3310 adhami3310 requested changes

@picklelo picklelo Awaiting requested review from picklelo

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KronosDev-Pro @picklelo @adhami3310