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

react-leaflet-draw updated to v3 react-leaflet 3.0.0 #90

Merged
merged 4 commits into from
Feb 7, 2021

Conversation

DVGY
Copy link
Contributor

@DVGY DVGY commented Feb 2, 2021

Please merge my branch

@DVGY DVGY changed the title [Work in progress ] react-leaflet-draw updated to v3 react-leaflet 3.0.0 react-leaflet-draw updated to v3 react-leaflet 3.0.0 Feb 3, 2021
@DVGY DVGY mentioned this pull request Feb 5, 2021
Copy link

@agusterodin agusterodin left a comment

Choose a reason for hiding this comment

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

Two things I notice:

Stray console.log left behind and i don't think the library author will be happy that the quotes all got changed from single to double. Run eslint fix to take care of this so that it conforms to the linter setup he has defined.

@agusterodin
Copy link

To be clear: not just the double quotes. I am guessing you ran this through a tool like prettier. It creates a lot of 'noise' in the changes. If you do use a tool like prettier be sure to also eslint fix. Doing so will greatly increase the chance that the repo owner will accept your PR.

Thanks for your hard work. Much appreciated!

Copy link
Collaborator

@srghma srghma left a comment

Choose a reason for hiding this comment

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

Could you please remove extra logs or add useful message as it is done with already existing logs

example/edit-control.js Outdated Show resolved Hide resolved
example/edit-control.js Show resolved Hide resolved
src/EditControl.js Outdated Show resolved Hide resolved
src/EditControl.js Outdated Show resolved Hide resolved
@DVGY
Copy link
Contributor Author

DVGY commented Feb 7, 2021

Could you please remove extra logs or add useful message as it is done with already existing logs

Yes, please I will take care of this. Give me some time and I will respond with all the new changes

@DVGY
Copy link
Contributor Author

DVGY commented Feb 7, 2021

To be clear: not just the double quotes. I am guessing you ran this through a tool like prettier. It creates a lot of 'noise' in the changes. If you do use a tool like prettier be sure to also eslint fix. Doing so will greatly increase the chance that the repo owner will accept your PR.

Thanks for your hard work. Much appreciated!

I removed all the double quotes

@srghma srghma merged commit a7ca8a8 into alex3165:master Feb 7, 2021
@DVGY DVGY mentioned this pull request Apr 16, 2021
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.

None yet

3 participants