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

feat: activate the content section of tailwind.config.js #256

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

siesdart
Copy link
Contributor

Description

Currently, in japr_tailwind package, the content section of the tailwind.config.js file is completely ignored during the build process, and only the pre-specified dart file path is used for the build. However, in actual use, it may sometimes be necessary to additionally register other files as content in addition to the dart file. Therefore, if there is no tailwind.config.js file, it automatically uses the pre-specified dart file path as now, but if there is a configuration file, I think it makes more sense to apply the content section in the file. Therefore, I modified the code accordingly and suggest this.

Type of Change

✨ New feature or improvement

Ready Checklist

  • I've read the Contribution Guide.
  • In case this PR changes one of the core packages, I've modified the respective CHANGELOG.md file using
    the semantic_changelog format.
  • I updated/added relevant documentation (doc comments with ///).
  • I added myself to the AUTHORS file (optional, if you want to).

@siesdart siesdart requested a review from schultek as a code owner July 23, 2024 00:25
Copy link

Package Version Report

The following packages have been updated:
jaspr : 0.13.3 -> 0.14.0
jaspr_builder : 0.13.3 -> 0.14.0
jaspr_cli : 0.13.3 -> 0.14.0
jaspr_flutter_embed : 0.3.2 -> 0.3.3
jaspr_riverpod : 0.3.12 -> 0.3.13
jaspr_router : 0.4.2 -> 0.4.3
jaspr_serverpod : 0.3.1+1 -> 0.3.2
jaspr_tailwind : 0.2.0 -> 0.3.0
jaspr_test : 0.13.3 -> 0.14.0

Copy link
Owner

@schultek schultek left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is a good change.

It should be mentioned in the documentation though that now if you define a custom tailwind config you have to set the content option. Please add this to both the jaspr_tailwind readme and the docs/eco/tailwind docs.

packages/jaspr_tailwind/lib/builder.dart Show resolved Hide resolved
@siesdart
Copy link
Contributor Author

@schultek As you mentioned, the document has been updated. However, since I am not from an English-speaking country, there is a possibility that there may be awkwardness in the sentences, so I would appreciate it if you could review it. Thank you.

Comment on lines 34 to 35
var assets = await buildStep.findAssets(Glob('{lib,web}/**.dart')).toList();
await Future.wait(assets.map((a) => buildStep.canRead(a)));
Copy link
Owner

Choose a reason for hiding this comment

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

When using a custom config with other content files, the tailwind command won't be rerun when those other files change. Do you think this is a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it may be a bit cumbersome to use, but since the main files are dart files, I don't think it's a serious problem. If you were to modify this part, you would have to read the config js file and extract the content part in dart. Is it possible to implement this?

Copy link
Owner

Choose a reason for hiding this comment

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

I think its ok for now, but a welcome improvement for a future PR if you want. So I'm going to merge this now and also add a hint that this isn't watching any additional files added to content.

Comment on lines 34 to 35
var assets = await buildStep.findAssets(Glob('{lib,web}/**.dart')).toList();
await Future.wait(assets.map((a) => buildStep.canRead(a)));
Copy link
Owner

Choose a reason for hiding this comment

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

I think its ok for now, but a welcome improvement for a future PR if you want. So I'm going to merge this now and also add a hint that this isn't watching any additional files added to content.

@schultek schultek merged commit a39d1f7 into schultek:main Aug 13, 2024
2 of 3 checks passed
@schultek
Copy link
Owner

Thanks again for your work on this, its really appreciated.

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.

2 participants