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

Closes #9871: Flex Topbar #10056

Merged
merged 4 commits into from
May 31, 2017
Merged

Conversation

IamManchanda
Copy link
Contributor

@IamManchanda IamManchanda commented May 21, 2017

This closes #9871.
The new docs codepen example already fixes this.

This just adds

  • Titlebar tweaks to make it look similar to float one
  • Add a test case for new docs example!
  • Docs update with new codepen example

Here is the flex version codepen (with titlebar tweaks) => https://codepen.io/IamManchanda/pen/OmBVZE?editors=1100

@IamManchanda
Copy link
Contributor Author

@kball ,,,, i think the docs update should go in master (live) also!

@kball
Copy link
Contributor

kball commented May 22, 2017

@rafibomb can you take a look at this?

@@ -42,7 +42,7 @@ $titlebar-icon-spacing: 0.25rem !default;

@if $global-flexbox {
display: flex;
justify-content: space-between;
justify-content: flex-start;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why this should be start (left align)

By default I think it should be space-between is you had 2 sides like:
http://foundation.zurb.com/sites/docs/off-canvas.html#combining-with-title-bar

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, this component can be manipulated with the flex helper classes. I think title bar should get it's own docs page.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I see when you dont use title bar left or right then flex start makes sense. I've added to your test case to demonstrate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So shall we insert this in a variable ?
@rafibomb?
and keep flex start?
And some docs about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On title bar docs, yes I agree infact having same discussion about it at #9991

@IamManchanda
Copy link
Contributor Author

@rafibomb @kball Awaiting reply for above
Can we close this before 6.4 RC?

@rafibomb
Copy link
Member

I don't know how I feel about removing the all in one responsive toggle docs example - it's nice to have but I get it needs work for flex version

@rafibomb
Copy link
Member

rafibomb commented May 31, 2017

It looks good from a bug fix perspective - merging!

What I'd like to see in the future:

  • title-bar has it's own docs page within navigation
  • title-bar inherits top-bar styles (height, color, background)
  • or eliminate the thing altogether and use top-bar on mobile and larger

Thoughts? @IamManchanda @kball @brettsmason @andycochran @colin-marshall

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.

6.3.0 Advanced top-bar with flex-box enabled causes the responsive-menu to render to the right
3 participants