Skip to content

Navbar fix for FireFox - #179

Merged
rhyneav merged 5 commits into
papercss:developfrom
Phreshhh:feature-navbar-fix-in-firefox
May 5, 2019
Merged

rhyneav merged 5 commits into
papercss:developfrom
Phreshhh:feature-navbar-fix-in-firefox

Conversation

@Phreshhh

Copy link
Copy Markdown
Contributor

Brief description

Navbar fix for Firefox.

Comment thread src/components/_navbar.scss Outdated
.collapsible {
@include resp(small) {
width: 100%;
margin-top: 50px;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the tests to pass, you need to move-up the margin-top property before the width: 100%.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OMG.. give me a sec. 😈

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can see what is wrong with the tests by clicking on the red-cross and then clicking on Travis Ci details.

You will see in the console output a detailed error thrown by Stylelint:

image

@Phreshhh

Copy link
Copy Markdown
Contributor Author

Finally!

Sorry about this, but the stylelint test not show error to me..

Thanks Totomlnc! Best regards.

@TotomInc

TotomInc commented Jan 29, 2019 •

Copy link
Copy Markdown
Contributor

@Phreshhh there is a small problem, the navbar seems to take too much height when not opened. This is happening on Chrome, Firefox and Safari, other than that everything works well.

Before:

capture d ecran 2019-01-29 a 15 17 53

After:

capture d ecran 2019-01-29 a 15 17 48

After investigation, this is due to the div.collapsible element which is taking the space, it seems to be due to the margin-top: 50px.

Also you can notice the burger menu button is missing the border.

@TotomInc

Copy link
Copy Markdown
Contributor

@Phreshhh next step is to add back the border on the burger button.

@TotomInc
TotomInc requested a review from rhyneav January 29, 2019 15:24
@TotomInc

Copy link
Copy Markdown
Contributor

Seems good for me! Waiting for @rhyneav review, don't worry this review can take some days. It will be merged by rhyneav. Thanks again for contributing to PaperCSS.

@Phreshhh

Copy link
Copy Markdown
Contributor Author

All feature bugs cleared. Thanks me too for the framework ;)

@rhyneav

rhyneav commented Feb 1, 2019 •

Copy link
Copy Markdown
Member

Hey @Phreshhh thank you for the PR! What exactly was this fixing in Firefox?

I also saw that you had changed the markup from a button to a label. This is fine since it fixes the bug, but I do think that we should keep the button selector in the LESS as well. This way it is still backwards compatible with anyone using the navigation with abutton. What do you think?

EDIT: Oops, forgot we use SCSS now...
EDIT 2: It fixes #173... oops!

@rhyneav rhyneav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See comment above, but thank you again for the contribution!

@Phreshhh

Phreshhh commented Feb 1, 2019

Copy link
Copy Markdown
Contributor Author

Hi @rhyneav !

I thinked about it when i changed, but i did't see other solution for fix it then change the markup. :(

I think either-or.

@rhyneav

rhyneav commented Feb 8, 2019

Copy link
Copy Markdown
Member

I think you can leave both with maybe a comment about why both are there. It's not the prettiest thing, but is safe until we launch the next major version.

.collapsible > button, // Leaving for backwards compatibility. See docs for proper usage
.collapsible > label {
  // those pretty styles
}

@Phreshhh

Copy link
Copy Markdown
Contributor Author

I put back the button styles, now works with old layout also same as before.
The old (with button) is not works FireFox but works in Edge and Chrome and the new (without button) works in all browser.

@rhyneav

rhyneav commented May 5, 2019

Copy link
Copy Markdown
Member

Thank you for updating!

@rhyneav
rhyneav merged commit 84dde0d into papercss:develop May 5, 2019
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.

3 participants