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

command runs "per-project" #42

Merged
merged 8 commits into from
Feb 7, 2023

Conversation

tyler36
Copy link
Collaborator

@tyler36 tyler36 commented Feb 3, 2023

This PR moves the browsersync command to a "per-project" level.

The command should not be available if the project doesn't support ddev-browsersync.

@tyler36 tyler36 force-pushed the tyler36/20230203-per-project-browsersync branch from b7b0b30 to a49d40d Compare February 3, 2023 03:34
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Looks good to me. Project also overrides global, so shouldn't matter that we've left an orphaned one. But... I wonder if it would be worth trying to delete the global one in post_install? That also seems intrusive.

@tyler36
Copy link
Collaborator Author

tyler36 commented Feb 3, 2023

I wonder if it would be worth trying to delete the global one in post_install? That also seems intrusive.

I thought about that. Most people wont even notice the change if/when they update.
People will definately notice if we deletes their files.

We could add a line to the README or maybe to the output on install?

@rfay
Copy link
Member

rfay commented Feb 3, 2023

Agreed - would be outstanding (and far better) to tell them what they can delete and why. For extra credit, test to see if it exists and if it does tell them to delete it.

@tyler36
Copy link
Collaborator Author

tyler36 commented Feb 3, 2023

Added first draft of note.

I suspect @mattstein could improve on it.

@rfay
Copy link
Member

rfay commented Feb 3, 2023

I don't see the first draft @tyler36

@tyler36
Copy link
Collaborator Author

tyler36 commented Feb 6, 2023

Pushed to correct branch.

Copy link
Contributor

@mattstein mattstein left a comment

Choose a reason for hiding this comment

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

Added suggestions for paring this down a little! First time I’ve looked at this readme and I may need to follow on with a PR that fixes a bunch of product names and smaller nitpicks that were well out of scope here.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tyler36 and others added 5 commits February 7, 2023 08:56
Co-authored-by: Matt Stein <m@ttste.in>
Co-authored-by: Matt Stein <m@ttste.in>
Co-authored-by: Matt Stein <m@ttste.in>
Co-authored-by: Matt Stein <m@ttste.in>
Co-authored-by: Matt Stein <m@ttste.in>
@tyler36
Copy link
Collaborator Author

tyler36 commented Feb 6, 2023

@mattstein Thank you!

@tyler36 tyler36 requested a review from mattstein February 7, 2023 00:05
@tyler36 tyler36 merged commit b24e8e8 into ddev:main Feb 7, 2023
@tyler36 tyler36 deleted the tyler36/20230203-per-project-browsersync branch February 7, 2023 07:32
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.

3 participants