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

bug fix: Fixes #23926 responsive state on navbar #23929

Merged
merged 1 commit into from
Sep 13, 2017

Conversation

andresgalante
Copy link
Collaborator

@andresgalante andresgalante commented Sep 12, 2017

#23652 fixes IE10 but introduces a bug on small devices on navbar on any browser, you can test it on v4-dev branch.

This PR closes #23926.

I've tested on IE10 up and other modern browsers.

@XhmikosR @mdo can you please review it?

@XhmikosR
Copy link
Member

XhmikosR commented Sep 12, 2017 via email

@Fouzyyyy
Copy link

Hi @andresgalante,

Just tested the pushed code, there is an issue with the Firefox browser v54.0, Chrome 60.0.3112.113, in the responsive mode (900 * 480) for instance. It's the same behavior reported in #23926

Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

@mdo: please have a look too; I confirmed it fixes the regression and also works on IE

@andresgalante
Copy link
Collaborator Author

andresgalante commented Sep 12, 2017

@Fouzyyyy you are right, the issue persists on the examples on the docs but not on the examples/jumbotron/ example.

My fix needs to be related to the breakpoint it expands as seen here /examples/navbars/

I'll update the PR. (IE10 💩 )

@mdo
Copy link
Member

mdo commented Sep 13, 2017

@andresgalante Do you have an update here for the fix? If it'll take some time, I'm probably going to revert the original PR instead to fix the newest bug.

@andresgalante
Copy link
Collaborator Author

andresgalante commented Sep 13, 2017

@mdo Yes, just updated it, its ready to merge now. Don't revert the original PR since we kinda need it to both kill IE10 bug and make this solution work. Sorry for the delay, I went for dinner and just got back to it 😝

@XhmikosR and @Fouzyyyy Thanks a lot for reviewing this PR and helping out finding the problems

@mdo mdo merged commit 5becfa6 into twbs:v4-dev Sep 13, 2017
@mdo
Copy link
Member

mdo commented Sep 13, 2017

Weirdly I was building files locally and accidentally pushed before hitting submit on my review. Thanks, @andresgalante!

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