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

Windows Docker and Tailwind watch mode #57

Merged
merged 4 commits into from
May 24, 2024
Merged

Windows Docker and Tailwind watch mode #57

merged 4 commits into from
May 24, 2024

Conversation

clussiana
Copy link
Contributor

Closes #56

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

I think we should repeat the same behavior like Tailwind does, i.e. --watch and --poll should be 2 separate options. IMO it would be better UX. You try the --watch first which will work in most cases, but if somehow it's not working for you - add that extra --poll option. First if all, it will be the same way as it's implemented upstream in Tailwind, so it will be intuitive, and second - it will avoid duplicating --watch when you run the command with --watch --pollWatch at the same time.

Could you refactor this please?

bocharsky-bw
bocharsky-bw previously approved these changes May 24, 2024
Copy link
Member

@bocharsky-bw bocharsky-bw 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 good to me now, thanks for adding more changes!

One minor question I would like to discuss... but I think we can even merge it as is

src/Command/TailwindBuildCommand.php Outdated Show resolved Hide resolved
bool $minify,
): Process {
$binary = $this->createBinary();
$arguments = ['-c', $this->configPath, '-i', $this->inputPath, '-o', $this->getInternalOutputCssPath()];
if ($watch) {
$arguments[] = '--watch';
if ($poll) {
$arguments[] = '--poll';
}
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use --poll without --watch? I see these fields are probably dependent, but this implementation will just silently not add --poll if no --watch was passed to the final command. But probably that's not a problem, because if you don't pass --watch then it's just finish the execution and that's it, so probably that's not a big deal to have this nested ot the --watch.

Just curious about your opinion on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, it should not be possible to use the --poll option if the --watch option is not called, maybe we should throw an exception if this case happens ?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, that's not something that Tailwind does, so I think it's safe to merge it as is.

Thanks again!

Co-authored-by: Victor Bocharsky <bocharsky.bw@gmail.com>
@bocharsky-bw bocharsky-bw merged commit 61d6d35 into SymfonyCasts:main May 24, 2024
11 checks passed
@clussiana clussiana deleted the 56-poll-option branch May 24, 2024 13:31
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.

Windows Docker and Tailwind watch mode
2 participants