Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Master fix#7 #13

Merged
merged 2 commits into from
Feb 21, 2018
Merged

Master fix#7 #13

merged 2 commits into from
Feb 21, 2018

Conversation

YuraDubensky
Copy link

@YuraDubensky YuraDubensky commented Feb 21, 2018

Fix for issue #7 and fix compatibility with latest boostrap.

Closes #7.

Fix issue Morgul#7 (Morgul#7)
Fix compatibility with latest Boostrap 4 (parent element should also has open class set)
@Morgul Morgul merged commit 6cda57a into Morgul:master Feb 21, 2018
@Morgul
Copy link
Owner

Morgul commented Feb 21, 2018

Looks great, thanks!

@YuraDubensky
Copy link
Author

May I ask you when next release is planned?

@Morgul
Copy link
Owner

Morgul commented Feb 22, 2018

@YuraDubensky My vague plan is "soon". I want get your other merge request in, and fix #12 before releasing a new one.

Regarding this change; I had not pulled it to run it locally until after it was merged, but there seems to be a few issues: the linter died on a tabs vs spaces issue (easy fix), but then most of the dropdowns on the demo page stopped working. (The ones appending to body continue to work.) Is there a chance you could look at that? It seems to have been introduced by your change; I'm hesitant to make a release until that's resolved.

@YuraDubensky
Copy link
Author

@Morgul Give me some time to setup demo page and try it out. But don't think my fix broke it, since it is working fine in my project with those changes. But I will verify and revert

@YuraDubensky
Copy link
Author

@Morgul Are using 2 spaces indentation in sources?

@Morgul
Copy link
Owner

Morgul commented Feb 22, 2018

...yes. (I'm not happy about it either, this is a fork of the original, and I haven't had time to redo their build system, so we're stuck with the original formatting decisions.)

My recommendation is to run grunt before doing a merge request. That will run all unit tests, and linting.

@Morgul
Copy link
Owner

Morgul commented Feb 22, 2018

Also, @YuraDubensky, I don't want a revert; your change is a very good one. It's entirely possible the dropdowns are wrong on the demo page. I just want the make sure the demo page is working, and there wasn't any unintended side effects. :)

@YuraDubensky
Copy link
Author

@Morgul I have fixed eslint errors for my 2 fixes and created a new pull request
#16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown
2 participants