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

Simplified positioning for the menu-panel - works for LTR and RTL layouts #3705

Closed

Conversation

scossar
Copy link
Contributor

@scossar scossar commented Aug 29, 2015

@eviltrout this seems straightforward. Maybe there's something I'm missing.

  • adds position: relative to .wrap
  • moves menu components in header.hbs into the wrap div
  • sets the horizontal position of .menu-panel absolutely in relation to its containing wrap div
  • removes horizontal positioning from menu-panel.js.es6

This works without any further adjustments for RTL layouts. With the way the menu-panel is currently being positioned, to position it correctly for RTL layouts will require checking if we are in a RTL layout and then adjusting the horizontal positioning values in menu-panel.js.es6.

From my point of view, as someone who wants to be able to heavily customize the Discourse css, it would be preferable if the menu components could be moved so that where they appear in the document relates to where they are positioned on the screen. This is immediately after the header <ul class='icons clearfix' role='navigation'></ul>. If you can do this, all of the changes in this PR will still work. There will no longer be a need to add position: relative to .wrap as it is already set on the .contents div.

As an added bonus, moving the components to their correct position in the document will allow you to set most of the vertical positioning of the menu without using javascript. (There is some difficulty in setting the slide-out menu vertical positioning and height without javascript as its position aligns with the main-outlet, not the header.)

@discoursebot
Copy link

You've signed the CLA, scossar. Thank you! This pull request is ready for review.

@scossar scossar force-pushed the simplified-menu-panel-positioning branch from a681035 to 5068303 Compare August 29, 2015 19:31
@scossar scossar force-pushed the simplified-menu-panel-positioning branch from 5068303 to 8d23b5d Compare August 30, 2015 15:40
@scossar
Copy link
Contributor Author

scossar commented Sep 1, 2015

The smallest change that you could make to allow the new menus to work for RTL is to move the menu components into the .wrap div.

<div class='wrap'>
    {{user-menu visible=userMenuVisible}}
    {{hamburger-menu visible=hamburgerVisible showKeyboardAction="showKeyboardShortcutsHelp"}}
    {{search-menu visible=searchVisible}}
  <div class='contents clearfix'>
    {{home-logo minimized=showExtraInfo}}
    {{plugin-outlet "header-after-home-logo"}}
    ...

That won't affect anything you have done, but will allow the RTL layout to be fixed with css.

@scossar
Copy link
Contributor Author

scossar commented Sep 5, 2015

I have improved on this quite a bit. I'll send it as a new PR.

@scossar scossar closed this Sep 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants