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.

Sign up for GitHub

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

Jump to bottom

Fix dropup in navbar and the caret position accordingly #23520

Merged
merged 3 commits into from
Oct 4, 2017
Merged

Fix dropup in navbar and the caret position accordingly #23520

merged 3 commits into from
Oct 4, 2017

Conversation

Johann-S
Copy link
Member

@Johann-S Johann-S commented Aug 17, 2017 •
edited by XhmikosR
Loading

We choose to disable Popper.js style for Dropdown in navbar but we didn't handle the case when we have a dropup in a bottom navbar and the change of the dropdown caret

  • Add documentation example

Fixes #23429
Fixes #22786

@Johann-S Johann-S requested a review from mdo August 17, 2017 14:36
@wojtask9 wojtask9 mentioned this pull request Aug 19, 2017
Closed
@mdo
Copy link
Member

mdo commented Aug 20, 2017

Can you take a stab at adding a docs demo for this with a fixed bottom navbar?

@Johann-S
Copy link
Member Author

Sure I'll take time to give some love to this PR 😄

@mdo
Copy link
Member

mdo commented Aug 22, 2017

Also, what about #22786?

@Johann-S
Copy link
Member Author

I added an example 😉 I'll take a look at this issue

@Johann-S Johann-S changed the title Fix dropup in navbar Fix dropup in navbar and the caret position accordingly Aug 23, 2017
@Johann-S
Copy link
Member Author

@mdo this PR fix now the issue you mentionned 👍 I think I've done my work on this PR if you have time to review my CSS before merging this one that's would be great 😄

houndci-bot
houndci-bot reviewed Oct 3, 2017
View reviewed changes
scss/_navbar.scss Outdated

.dropdown-toggle {
&::after {
border-top: 0;
Copy link

Choose a reason for hiding this comment

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

Properties should be ordered border-bottom, border-top

scss/_navbar.scss Outdated
}

.dropdown-toggle {
&::after {
Copy link

Choose a reason for hiding this comment

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

Selector should have depth of applicability no greater than 2, but was 4
Nesting should be no greater than 4, but was 5

@Johann-S Johann-S force-pushed the fixDropupInNavbar branch 2 times, most recently from 3952292 to 2cc1212 Compare October 4, 2017 08:03
@Johann-S
Copy link
Member Author

Johann-S commented Oct 4, 2017

If you found something wrong here in my CSS @mdo and @andresgalante feel free to correct it 😄 I'll merge this PR for now

@Johann-S Johann-S requested a review from XhmikosR October 4, 2017 08:12
@twbs twbs deleted a comment from houndci-bot Oct 4, 2017
@twbs twbs deleted a comment from houndci-bot Oct 4, 2017
@twbs twbs deleted a comment from houndci-bot Oct 4, 2017
@twbs twbs deleted a comment from houndci-bot Oct 4, 2017
@twbs twbs deleted a comment from houndci-bot Oct 4, 2017
@XhmikosR
Copy link
Member

XhmikosR commented Oct 4, 2017

@Johann-S: the dropup in the example still opens to the bottom.

@Johann-S
Copy link
Member Author

Johann-S commented Oct 4, 2017

I tested that a few minutes ago and the dropup in the example opens to the top 🤔

@Johann-S
Copy link
Member Author

Johann-S commented Oct 4, 2017 •
edited
Loading

Hmm no sorry I just checked an it opens to the top :

image

Make sure you checked on that URL : http://localhost:9001/docs/4.0/examples/navbar-bottom/

@XhmikosR
Copy link
Member

XhmikosR commented Oct 4, 2017

OK, it's because of the dist files. It seems to work fine.

@Johann-S Johann-S merged commit a9e7abd into v4-dev Oct 4, 2017
@Johann-S Johann-S deleted the fixDropupInNavbar branch October 4, 2017 08:32
@mdo mdo mentioned this pull request Oct 4, 2017
Closed
wojtask9
wojtask9 reviewed Oct 4, 2017
View reviewed changes
scss/_navbar.scss
@@ -74,6 +74,13 @@
position: static;
float: none;
}

.dropdown-toggle {
Copy link
Contributor

Choose a reason for hiding this comment

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

For me this change is unnecessary. Why we need redefine .dropdown-toggle in .navbar?

Copy link
Member Author

Choose a reason for hiding this comment

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

because without that we cannot have the up caret

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested and works without these lines.
Still I don't understand why this is needed :)

wojtask9
wojtask9 reviewed Oct 4, 2017
View reviewed changes
scss/_navbar.scss
@@ -144,6 +151,18 @@
padding-right: 0;
padding-left: 0;
}

.dropup {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @Johann-S but here is another issue.
This should be moved to media-breakpoint-up because currently it only works with .navbar-expand but not with .navbar-expand-{sm,md}

Copy link
Member Author

Choose a reason for hiding this comment

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

you're right ! Can you open an issue which explain that ? Or a PR if you want to fix that ? 👍

@Johann-S Johann-S mentioned this pull request Oct 4, 2017
Merged
@wojtask9 wojtask9 mentioned this pull request Oct 4, 2017
Closed
@tmorehouse tmorehouse mentioned this pull request Oct 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojtask9 wojtask9 wojtask9 left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@XhmikosR XhmikosR XhmikosR approved these changes

@mdo mdo Awaiting requested review from mdo

@andresgalante andresgalante Awaiting requested review from andresgalante

Assignees
No one assigned
Labels
css v4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Johann-S @mdo @XhmikosR @wojtask9 @houndci-bot