[removed] Remove lodash dependency and hardcoded label

[bfink13]

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.

[CMTegner]

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?

[bfink13]

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

[bfink13]

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

[CMTegner]

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]

Thanks for the opportunity :)

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

[CMTegner]

Trailing whitespace?

[CMTegner]

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]

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.

[CMTegner]

labelText still needs removing from propTypes and defaultProps.

[CMTegner]

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]

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

[bfink13]

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

[CMTegner]

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

[bfink13]

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

[akhaku]

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]

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]

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]

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.

[CMTegner]

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

Blockers

• Dependency cleanup #94

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

• [fixed]: check item length before calling scrollinto view #52
• [fixed] Prevent autocomplete when clicking a non-focusable input #68
• [fixed] Select highlighted item on input click #84
• [changed] Prevent Return in menu from submitting surrounding form #92
• [changed] Disable native autocomplete #93
• Add getItemValue to propTypes #103
• renderMenu prop #104
• fix arrow up/down bug when list is empty #106

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

• Menu styling/positioning: The current implementation and default setup is not great. I'm tempted to just ditch it all and force the user to decide how to render the menu (we'll provide offset and other meta data). The alternative is to go for a relatively positioned menu, which we could easily do since we wrap both the input and the menu with a div. I could mock up both suggestions unless you have a better suggestion. Here are some issues for reference: Call setMenuPositions() on scroll & resize #69, Use 'absolute' positioning for menu instead of 'fixed' #70, [changed]Set the position of menu as "absolute" instead of "fixed" #75, Support mobile safari's broken offsets #100, Add default z-index to menu style #102
• highlightedIndex: [fixed]: check item length before calling scrollinto view #52 and fix arrow up/down bug when list is empty #106 try to fix some of the issues, but the fact is that highlightedIndex isn't updated when props.items changes. This needs to be fixed properly.

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

• only focus ComboboxOption elements #13
• Added browserify directive to make sure that JSX->JS transform happens w... #15
• remove jsx pragma (React 0.13 compat) #30
• Make getItemValue required #45
• [added] wrapperProps and wrapperStyle props #50
• [fixed] Add missing event argument to ArrowDown handler #53
• ArrowDown() needs event argument #64
• Issue41 #74
• Include an 'id' prop #95
• new build in order for wrapperStyles and wrapperProps to work as expected #97
• Support React v15 #99
• Enable adding label properties #105
• + more, these were just the obvious ones