darrenjennings / vue-autosuggest

🔍 Vue autosuggest component.
https://darrenjennings.github.io/vue-autosuggest
MIT License
621 stars 91 forks source link

[feature request] Make vue-autosuggest fully compatible with BEM #96

Closed wujekbogdan closed 5 years ago

wujekbogdan commented 5 years ago

It's great that vue-autosuggest follow the BEM convention, but it would be even greater if it followed the BEM convention fully ;)

darrenjennings commented 5 years ago

I'm going to be honest with you that I have never actually worked with BEM before so I'm actually quite surprised it's so close to BEM convention. Close, but no cigar.

This looks like a good PR for outsiders who don't necessarily need to understand the codebase. PR'd against the 2.0 branch if you were up for contributing. If you're interested, a PR with BEM standards, as well as prefix customization would be welcome!

wujekbogdan commented 5 years ago

@darrenjennings I'll create a PR soon.

scottadamsmith commented 5 years ago

Can I play devil's advocate here? I wonder if the missing feature is just to be able to override two things:

By providing these two features and not changing the existing class names, consumers will have the ability to define classes that follow BEM convention or any other convention they choose, while not introducing a breaking change for existing users.

I understand that this change would be in a major release and breaking changes are acceptable, but I still think minimizing them when possible is ideal. If the belief is that the majority of consumers want BEM classes, then the change makes sense. But if only a few do, this is a breaking a change that only really benefits a few so that they won't have to provide their classes via configuration.

darrenjennings commented 5 years ago

@scottadamsmith thanks for the feedback, always appreciated.

Class prefix customization will be fixed by #97. Originally, I did not want to add any classnames on any element, but the "rubber to the road" truth was that I needed them for query selecting and accessibility. I believe this to still be an acceptable architecture, but ideally we need to solve use cases like users coming from bootstrap, or tailwind. I don't like prop explosion so I'm less likely to go with solutions like #90.

If the belief is that the majority of consumers want BEM classes, then the change makes sense. But if only a few do, this is a breaking a change that only really benefits a few so that they won't have to provide their classes via configuration.

This lib is small enough that I'm willing to "break" more things, so that new users can benefit, while alleviating the punishment on existing users with a nice changelog/migration guide. My goal is to break as many features as possible in 2.0 in an attempt to make it more intuitive for new users, while (hopefully) increasing the longevity of vue-autosuggest.

Going forward, I think we should accept BEM defaults, allow to customize via prefix and append new classnames via v-bind props, similar to how the <input class works: https://github.com/darrenjennings/vue-autosuggest/blob/b825be24211450da3700898ade30701ecdd3b491/src/Autosuggest.vue#L8

darrenjennings commented 5 years ago

Closed by 2.0.0

Was an oversight now that I'm reviewing, I did not add ability for class to be added <ul>. Would accept a PR but closing this ticket since it relates to BEM, not new classes.