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

Inline SCSS (close #158) #160

Merged
merged 2 commits into from
Apr 13, 2021
Merged

Inline SCSS (close #158) #160

merged 2 commits into from
Apr 13, 2021

Conversation

Tanja-4732
Copy link
Contributor

@Tanja-4732 Tanja-4732 commented Mar 31, 2021

This PR is supposed to close #158.

Checklist

@Tanja-4732 Tanja-4732 marked this pull request as ready for review March 31, 2021 19:52
@Tanja-4732
Copy link
Contributor Author

b2640aa reverts the version bump back to 0.10.0 as requested at https://discord.com/channels/793890238267260958/793890238267260963/828700547972333619

Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

@Bernd-L thank you for taking the time to put this PR together! I hope my review response is not too overwhelming. It is not easy to communicate this via text, but all of my review here is in good heart!

Overall, I would say 95% of this PR is not actually related to the one thing this PR "should" be about. For that last 5%, it is well-crafted.

For future PRs, my primary advice would be to start small. Do the absolute minimum to solve your problem. If you really want to write more code and refactor more stuff (which I totally understand), please do so in a separate PR ... and typically it would be a good idea to ask the maintainers if such is even welcome. Style is subjective. Naming is subjective. I'm sure you take my point.

The reasoning behind my recommendation here is that the cognitive load gets higher and higher as changes are added. This can lead to bugs and other such undesired behavior as review becomes more difficult. Ping me for another round of review when you are ready.

Cheers! Let me know if you have any questions! Happy hacking.

.vscode/settings.json Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
examples/yew/Cargo.toml Outdated Show resolved Hide resolved
src/pipelines/sass.rs Outdated Show resolved Hide resolved
src/pipelines/sass.rs Show resolved Hide resolved
src/pipelines/sass.rs Show resolved Hide resolved
src/pipelines/sass.rs Outdated Show resolved Hide resolved
src/pipelines/sass.rs Outdated Show resolved Hide resolved
@thedodd
Copy link
Member

thedodd commented Apr 6, 2021

@Bernd-L also, very importantly, I would highly recommend that you squash all of your commits down to a single well-formed commit. A single title line describing the high-level of what you've done, followed by a more descriptive paragraph describing what you've done.

Better that you have control over that than me having to squash it all down before I merge.

@Tanja-4732
Copy link
Contributor Author

My latest commit f72448a aims to revert all undesired markdown changes but keep all spelling fixes and doc updates.

Let me know if you want further changes to the format reversal.

Allows for compiling and then in-lining SASS and SCSS using a new
data-inline attribute in a data-trunk link tag.

Compiling SASS and SCSS without inlining is still supported.

This commit also fixes several spelling mistakes and updates
documentation to reflect the changes.

A non-squash version can be found at Bernd-L:inline-scss_no-squash,
in case you want to see individual commits or for reverting
individual changes.
@Tanja-4732
Copy link
Contributor Author

@thedodd I have now squashed all my commits into one single commit.

Please note that a commit from @lukechu10 is also present, since it was made ~9 hours ago, and I couldn't squash it into this pull request.

I hope this PR is now good enough for a simple merge (without further squashing on your end).

My commit message reads as follows:

Add data-inline to SASS/SCSS (close #158)

Allows for compiling and then in-lining SASS and SCSS using a new
data-inline attribute in a data-trunk link tag.

Compiling SASS and SCSS without inlining is still supported.

This commit also fixes several spelling mistakes and updates
documentation to reflect the changes.

A non-squash version can be found at Bernd-L:inline-scss_no-squash,
in case you want to see individual commits or for reverting
individual changes.

If you need any further changes, let me know.
And please don't squash my commit, thanks!

@Tanja-4732 Tanja-4732 requested a review from thedodd April 6, 2021 12:49
thedodd
thedodd previously approved these changes Apr 13, 2021
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for all of the work here @Bernd-L, and thank you for responding kindly to my review comments!

I've left a few comments here, but I am just going to address them once I merge. Cheers!

README.md Show resolved Hide resolved
site/content/assets.md Show resolved Hide resolved
site/content/commands.md Show resolved Hide resolved
src/pipelines/mod.rs Show resolved Hide resolved
src/pipelines/sass.rs Show resolved Hide resolved
@Tanja-4732
Copy link
Contributor Author

@thedodd Dies this mean that my commit will be part of the main branch?

That'd be nice, I'm looking forward to the next release too.

@thedodd thedodd merged commit ef855d3 into trunk-rs:master Apr 13, 2021
@thedodd
Copy link
Member

thedodd commented Apr 13, 2021

@Bernd-L done.

@Tanja-4732
Copy link
Contributor Author

Thanks!

It's great to see my first contribution (to an actual codebase, not just Docs) get merged 😊🎉🚀

@thedodd
Copy link
Member

thedodd commented Apr 13, 2021

@Bernd-L grats! Thanks again for all of the hard work here.

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.

Compile AND inline SCSS using rel="scss" data-inline
3 participants