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

Issue #158 insertstyle node modules export docs #164

Merged
merged 6 commits into from
Oct 30, 2024

Conversation

elycruz
Copy link
Owner

@elycruz elycruz commented Sep 25, 2024

Cleans up docs related to using the insert feature.

Related to #157 and #158 .

- Simplified the usage notice
  and added link to 'package.json.files'
  docs.
- Simplified the usage notice
  and added link to 'package.json.files'
  docs.
@elycruz elycruz added enhancement docs Documentation issue. labels Sep 25, 2024
@elycruz elycruz self-assigned this Sep 25, 2024
@marcalexiei
Copy link
Collaborator

I opened #163 10 minutes ago.
When you have time consider to take a look.

It could solve the problem in a more "graceful" way 😄

@elycruz
Copy link
Owner Author

elycruz commented Sep 25, 2024

:-D, yeah I saw - I figured since I had updated the snippet I would added it for correctness/have it in the git history.

Started reviewing your PR though if we could get the required update on it's (apart from the prettier stuff) that would be better (as I don't get too much time for reviewing the PRs);

Let's try to go for PRs/branches that build off each other (if necessary) - will allow for PR reviews to be completed faster.

@marcalexiei
Copy link
Collaborator

Started reviewing your PR though if we could get the required update on it's (apart from the prettier stuff) that would be better

Prettier PR changes are nearly all related to code formatting, so they should't take long.
In addition I highlighted some unrelated changes using review comments to speed up the review 🙂.


if we could get the required update on it's (apart from the prettier stuff) that would be better

IMHO having a formatter and a linter (coming 🔜) ensure that the code is more readable and subject to errors,
so I consider prettier PR more important 😃


(I don't get too much time for reviewing the PRs)

If you wish you can add me as collaborator with the triage permission:
"Can read and clone this repository. Can also manage issues and pull requests"

I can't edit or write any information related to the repository code and settings.
More info can be found here

But with that permission at least I can at have CI checks running and avoid one review cycle just to fix some CI error 😅.

If you do not want to add me, no problem, I understand 😉.

@elycruz
Copy link
Owner Author

elycruz commented Oct 7, 2024

As to your previous comment, and adding you as 'triage' user, will look into it over the next coming days.

@marcalexiei
Copy link
Collaborator

Hi @elycruz, thanks for the invitation 🎉.
I see that you granted me the maintainer permission so I should be able to push to this repository 🙇.
However PRs require 1 approving review and I cannot approving my own PRs 😅.

If you don't have time to review the PR's,
I can turn around this by opening PRs just to check that the Cl is passing,
then cherry pick the commit(s) on main.

Let me know what is the most feasible solution for you!

Just a quick summary on the current 3 opened PR's if you have time to take a look:

  1. test(insertStyle): ensures that tests are run in sequence #165 - very simple, just a fix for the coverage drop due to my earlier test changes
  2. feat: output insertStyle as part of rollup bundle #163 - a more "clean" solution to avoid node_modules folder creation inside rollup output folder when using insert options (most of the changes are related to unit test)
  3. add api option to allow switching to sass modern compiler #166 - support for sass modern compiler (it's very big but I would highly appreciate your feedback on it)

@elycruz
Copy link
Owner Author

elycruz commented Oct 18, 2024

@marcalexiei hey, I'm merging this one for now - you can still make the edit you require in your insertStyle PR.

@marcalexiei
Copy link
Collaborator

@elycruz beware that CI is failing due to format issues

@marcalexiei
Copy link
Collaborator

you can still make the edit you require in your insertStyle PR.

insertStyle PR is ready from my point of view.
Merging this PR now will probably cause conflict that I will have resolve on my branch.

- Updated `insert` feature docs to reflect latest change of where rollup now outputs the 'insertStyle' module into a non-conflicting directory on build (e.g., 'dist/_virtual', etc).
@elycruz elycruz merged commit 4c8c457 into main Oct 30, 2024
6 checks passed
@elycruz elycruz deleted the issue-#158-__insertstyle_node_modules_export_docs branch December 27, 2024 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation issue. enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants