Skip to content
This repository has been archived by the owner on Aug 31, 2022. It is now read-only.

Cleanup #12

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Cleanup #12

wants to merge 6 commits into from

Conversation

aight8
Copy link

@aight8 aight8 commented May 26, 2016

  • Update babel and eslint to the latest version since they was very outdated
  • Fix all eslint errors/warning and adjust eslint configuration
  • Fix some other typos/missings
  • Update readme title
  • Remove content of lib
  • Attention: Removed deprecated code. Breaking change for this major version.

@rewop
Copy link

rewop commented May 26, 2016

@aight8 I believe it is not wise to merge already such a big change. I believe we need first to be able to run the module at least on the examples, and check that the tests are still valid.

Also as discussed with @cristiandley in #6 we should first agree on a roadmap, otherwise the development goes in all directions, and this is not what we want.

Finally this PR tries to achieve too much. 89 files changed is quite a lot to even try to review. I would take smaller and solid steps that are in line with the roadmap. Let's not waste our effort here! :)

@cristiandley what do you think?

@aight8
Copy link
Author

aight8 commented May 26, 2016

I removed the content of the lib folder so that's the most of the changes :) The other are small things (separated in commits) and bug fixes which eslint found in it's new configuration. The biggest thing is the removal of the deprecated method since this two-way solution was too confusing. I tested the map with basic configuration, that worked.

But I agree we should look for a roadmap as soon as possible.

<MarkerClustererCreator
markerClusterer={this.state.markerClusterer}
{...restProps}
>

Choose a reason for hiding this comment

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

I would deactivate in case active the rule react/jsx-closing-bracket-location, so closing brackets can be located everywhere.

@cristiandley
Copy link
Owner

cristiandley commented May 26, 2016

I like your energy @aight8 tell me, does it works ? did you test it ?

@rewop The 89 lines seems to be the delete of /lib folder. And yes next time make differents PR so we can review and merge it easy hehe 😄

Has ScriptJsLoader been deprecated ? please link it. Everything else looks cool. Lets try to follow the road map.

@rewop
Copy link

rewop commented May 27, 2016

Yes I noticed about the deletion of lib folder.
However having many files deleted still makes very difficult to review the rest in my opinion. That is why the changes here should be the work of many PRs, and one specifically only to remove the lib folder.

What I say is it is much easier to review and merge a PR for each item listed in the description on top, rather then merging one PR with all those changes together.

@cristiandley
Copy link
Owner

@rewop is right. There is no way to make a proper testing or evaluate such a big change.
@aight8 could you make little PRs ? just diff what you have. 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants