-
Notifications
You must be signed in to change notification settings - Fork 529
[removed] Remove lodash dependency and hardcoded label #89
Conversation
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. It seems like the 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 I say we remove the @bfink13 will you modify this PR, or do you want me to create one that supersedes yours? |
As discussed in reactjs#89
That's an excellent idea! I'll update the PR. |
As discussed in reactjs#89
@CMTegner saw you linked an update where you remove the label and lodash - do you want me to close this one? |
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 :) |
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 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace?
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. |
e2ae224
to
81c6471
Compare
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. |
return ( | ||
<div style={{...this.props.wrapperStyle}} {...this.props.wrapperProps}> | ||
<label htmlFor={this.id} ref="label"> | ||
{this.props.labelText} |
There was a problem hiding this comment.
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
.
The easiest way to squash is to
I prefer rebasing over merging master into my working branches, since it gives the least amount of commit noise. |
81c6471
to
5df1992
Compare
Alright I think this should actually be good to go now. Thanks for the tips! |
@CMTegner is there a schedule for the next release? When are all these open PRs getting pulled in? |
I'm sure the people with commit access will get around to merging this as soon as they have time :) |
LOL geez....with the amount you were participating I figured you were the guy :D my bad. |
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! |
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 |
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. |
Good stuff everyone. Sorry for the delays here, I'll get this merged in and publish |
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.