Skip to content
This repository was archived by the owner on Jul 19, 2019. It is now read-only.

[removed] Remove lodash dependency and hardcoded label #89

Merged
merged 1 commit into from
May 25, 2016

Conversation

bfink13
Copy link
Contributor

@bfink13 bfink13 commented Apr 3, 2016

Lodash is massive and quite unnecessary for generating something as simple as a unique ID. The minified build of react-autocomplete is 72kb (!!!!!!). Besides, consumers should be able (and I'd argue responsible) for supplying an ID.

The alternative to an ID was putting the input inside the label, but I figured that wouldn't be desirable given that it could possibly break some layouts.

@bfink13 bfink13 changed the title Remove lodash dependency and expose input id [added] Remove lodash dependency and expose input id Apr 3, 2016
@CMTegner
Copy link
Collaborator

CMTegner commented Apr 6, 2016

I am 100% for removing the lodash dependency. I <3 lodash, but including the entire library for one function is super overkill. We could include only the parts we need (i.e. import uniqueId from 'lodash/uniqueId'), but that's beside the main point: why are we rendering a label at all?

It seems like the <label> markup was added without any discussion around the subject, probably because it was baked into a PR which was really useful (adding tests).

The problem with adding more markup is that the consumers will inevitably want to change the way it's rendered, add props, do various hacks, which means we need to add more code, bloat the API, etc. (we already have this problem with the <div> that wraps the autocomplete input and menu, see #91).

I say we remove the <label> markup entirely, and let the consumer add an id via props.inputProps.id (to link it with their custom <label>).

@bfink13 will you modify this PR, or do you want me to create one that supersedes yours?

CMTegner added a commit to CMTegner/react-autocomplete that referenced this pull request Apr 6, 2016
@bfink13
Copy link
Contributor Author

bfink13 commented Apr 6, 2016

That's an excellent idea! I'll update the PR.

CMTegner added a commit to CMTegner/react-autocomplete that referenced this pull request Apr 6, 2016
@bfink13
Copy link
Contributor Author

bfink13 commented Apr 7, 2016

@CMTegner saw you linked an update where you remove the label and lodash - do you want me to close this one?

@CMTegner
Copy link
Collaborator

CMTegner commented Apr 7, 2016

Oh, I didn't realise it got cross referenced here. I just created that branch in case you didn't find the time to do it (I'm hoping we can get it merged before 1.0). If you want me to create a PR from it that's fine by me, I just didn't want to rob you of the chance to fix it :)

@bfink13
Copy link
Contributor Author

bfink13 commented Apr 10, 2016

Thanks for the opportunity :)

I just updated the PR. There were some conflicts with master on the wrapper div but nothing big.

@@ -313,11 +311,9 @@ let Autocomplete = React.createClass({
state: this.state
})
}
return (

return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trailing whitespace?

@CMTegner
Copy link
Collaborator

I think you can just squash all the commits in this PR. It makes it easier to see what actually happened without having having to understand the additional commits.

@bfink13
Copy link
Contributor Author

bfink13 commented Apr 10, 2016

Alright I think this is all sorted now. This is my first PR through Github, let me know if I need to do anything different with the squashing.

@bfink13 bfink13 changed the title [added] Remove lodash dependency and expose input id [added] Remove lodash dependency and hardcoded label Apr 10, 2016
@bfink13 bfink13 changed the title [added] Remove lodash dependency and hardcoded label [removed] Remove lodash dependency and hardcoded label Apr 10, 2016
return (
<div style={{...this.props.wrapperStyle}} {...this.props.wrapperProps}>
<label htmlFor={this.id} ref="label">
{this.props.labelText}
Copy link
Collaborator

Choose a reason for hiding this comment

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

labelText still needs removing from propTypes and defaultProps.

@CMTegner
Copy link
Collaborator

The easiest way to squash is to git reset --soft <ref>, ref being the point where you diverged from master. When you've been working on a branch for a while it tends to be a good idea to rebase, to make sure your branch doesn't contain any merge conflicts against master. What I usually do:

  • git rebase upstream master (I usually set upstream to point to the repo I forked from)
  • Fix any merge conflicts
  • git reset --soft <ref>
  • git commit -m 'A sensible commit message'

I prefer rebasing over merging master into my working branches, since it gives the least amount of commit noise.

@bfink13
Copy link
Contributor Author

bfink13 commented Apr 11, 2016

Alright I think this should actually be good to go now. Thanks for the tips!

@bfink13 bfink13 mentioned this pull request Apr 13, 2016
@bfink13
Copy link
Contributor Author

bfink13 commented Apr 19, 2016

@CMTegner is there a schedule for the next release? When are all these open PRs getting pulled in?

@CMTegner
Copy link
Collaborator

I'm sure the people with commit access will get around to merging this as soon as they have time :)

@bfink13
Copy link
Contributor Author

bfink13 commented Apr 19, 2016

LOL geez....with the amount you were participating I figured you were the guy :D my bad.

@akhaku
Copy link

akhaku commented May 3, 2016

Just want to add my +1 for this - ran into the issue fixed by #95 earlier, but followed the reference to this PR which is even better. Hope this gets merged into something I can fetch from npm soon!

@adamblank
Copy link

adamblank commented May 5, 2016

In addition to being unnecessary, including Lodash like this breaks browserified JS environments that use underscore.js. This is in part due to how Lodash works with Browserify. If Lodash isn't going to be removed from this project, it should at least use noConflict

@Davorak
Copy link

Davorak commented May 9, 2016

The unique id generation is also preventing me for easily doing some equality tests between pages generated in slightly different environments.

So one more vote and one more reason to merge this in.

@whichsteveyp
Copy link
Contributor

whichsteveyp commented May 25, 2016

Good stuff everyone. Sorry for the delays here, I'll get this merged in and publish 1.0.0-rc3, @CMTegner would like to get an idea of what else we should merge in and finalize for 1.0.0 too.

@whichsteveyp whichsteveyp merged commit 0b622ed into reactjs:master May 25, 2016
@CMTegner
Copy link
Collaborator

@sprjr good to have you back! Here's my take on v1:

Blockers

Nice-to-have (bugfixes, changes, improvements)

There's a couple of topics we need to address either before v1 or soon thereafter:

To be closed (already fixed, no longer applicable, etc)

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.

6 participants