Skip to content

Conversation

@Johann-S
Copy link
Member

Finally I revert this code, because there are a lot of use case to detect a valid uniq id, so it's better to add information about that in documentation

Copy link
Member

@glebm glebm left a comment

Choose a reason for hiding this comment

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

Thanks!

@XhmikosR
Copy link
Member

I'm personally thinking we might need to revert this change. I mean, technically we are right, but it is a breaking change...

That being said, I'd change this one to a warning or something to make it stand out maybe.

@Johann-S
Copy link
Member Author

That's not a breaking change @XhmikosR because that's what we shipped in our last release see: https://github.com/twbs/bootstrap/blob/v4.1.3/js/src/util.js#L78-L89 That's exactly the same code, we just add some information about valid selectors

@XhmikosR
Copy link
Member

Yes, I meant when this was added, not the revert.

@Johann-S
Copy link
Member Author

Oh ok 😄
Why not make that into a warning but I thought it was to huge to be a warning 🤔

@Johann-S Johann-S force-pushed the v4-dev-jo-selectors branch from aca3a42 to 2032e20 Compare August 27, 2018 18:25
@XhmikosR
Copy link
Member

@Johann-S: is it hard to revert the related changes? If it's too hard let's leave it as is and make this a warning. But ideally we shouldn't introduce breaking changes.

@Johann-S
Copy link
Member Author

Is it a bit hard, plus it'll reopen issues about perfs we closed thanks to that change

@Johann-S Johann-S force-pushed the v4-dev-jo-selectors branch from 2032e20 to e7ee333 Compare August 28, 2018 07:13
@XhmikosR
Copy link
Member

I don't know at this point. When was this introduced? 4.1.3 or older?

@Johann-S
Copy link
Member Author

It was on 4.1.2

@Johann-S Johann-S merged commit a3e45d8 into v4-dev Aug 31, 2018
@Johann-S Johann-S deleted the v4-dev-jo-selectors branch August 31, 2018 19:00
@mdo mdo mentioned this pull request Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants