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

Update Glup (and friends), along with Gulpfile.js and package.json #193

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ran-dall
Copy link
Contributor

@ran-dall ran-dall commented Jun 2, 2022

@EliverLara It's really hard to contribute to (and help folks with) the project in the state the workflow is in. So I decided to take some time this morning and fix the workflow, therefore I'm giving out the right information and making the right changes.

The whole Gulp process was broken because gulp-sass no longer has a sass processor but requires dart-sass (by preference). Beyond that (as you probably know from Dependabot PRs) package.json's dependencies were heavily out of date. Not to mention, that it was also missing a lot of basic information.

This commit brings the dependencies up-to-date and updates the package.json.

Closes #135 #128 #116 #115 #103 #202

@ran-dall
Copy link
Contributor Author

ran-dall commented Jun 2, 2022

@EliverLara While this commit fixes the workflow for me. I did want to point out that the code breaks compliance on dart-sass. If you are good with the idea, I would recommend we reprocess it with dart-sass which I believe pretty much is the same formatting as prettier. This can also be integrated with PostCSS (which now is more widely recommended than using sass; for web development, at least) which has integration to dart-sass and also prettier.

This may also address some of the trivial errors produced by gnome-shell as I'm pretty sure they use dart-sass upstream. At least, most people do since node-sass was discontinued.

I didn't want to break what you had or change it up too much without talking to you first. 🤝

@ran-dall
Copy link
Contributor Author

ran-dall commented Jun 2, 2022

@EliverLara I went ahead and updated this to work with gtk-4.0. I think I did it correctly; it's been a REALLY long time since I used Gulp, so I'm sure the Gulpfile.js could benefit from another look.

I fixed all the errors that dart-sass was having with the code. Most were pretty trivial.

@ran-dall
Copy link
Contributor Author

ran-dall commented Jun 17, 2022

@EliverLara I just rebased this to current, and it auto-closed. Reopened.

@ran-dall ran-dall reopened this Jun 17, 2022
@ran-dall
Copy link
Contributor Author

@EliverLara I would appreciate it if we can merge this soon, brother. FWIW, I've been using this, there is nothing that affects the use of the theme. Most of these changes made by dart-sass are trivial formatting fixes.

@ran-dall
Copy link
Contributor Author

@EliverLara I rebased everything and brought everything back to current.

@ran-dall
Copy link
Contributor Author

@EliverLara Pushed another update to keep it in line with master. Is there anything else holding this from being merged?

The package.json's dependencies were heavily out of date. Not to mention, that it was also missing a lot of basic information.

This commit brings the dependencies up-to-date and updates the package.json.
@ran-dall
Copy link
Contributor Author

ran-dall commented Aug 6, 2022

@EliverLara Hey brother, this PR has been stale for 2 months with no response. Is there a reason it hasn't been merged yet? Is there something I can fix or improve on to get this merged faster?

@EliverLara
Copy link
Member

Hi @ran-dall I'm sorry for the very late response, I haven't had enough free time to review all the files modified with this PR and test if everything works fine, I also need to check if unusable code has not been generated. That's the main reason i hadn't made the switch to dart-sass, I really appreciate your efforts, please give me some time to do those checks, I will try to do it ASAP.

@ran-dall
Copy link
Contributor Author

ran-dall commented Aug 8, 2022

@EliverLara Oh, okay. That makes sense.

FWIW, I've been using the output of this version for months, and haven't noticed anything.

That said, when you look, most of the changes revolve around the closing bracket }, which dart-sass wants on a new line. Most (if not all) of the changes relate to this.

I'll be drafting a new PR for #207; however, the problem there is deeper than what one would initially think. TBH it's the same problem we've been facing with GNOME 40+ (aka LibHandy/LibAdwaita changes). I'll send up that PR for you to review, with the offending code, commented out for your review.

There'll be more information about all this on that PR, but just a heads up, I'm basing that PR on this one, as this is the only way I can work on it ATM. Hope that's not an issue.

This commit fixes all immediate errors that would cause `dart-sass` to error out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants