Toggle navibar on mobile #190
Conversation
|
fix for #185 |
Codecov Report
@@ Coverage Diff @@
## master #190 +/- ##
=======================================
Coverage 76.56% 76.56%
=======================================
Files 33 33
Lines 973 973
=======================================
Hits 745 745
Misses 228 228 |
|
Apart from that 👍 |
I have same position like |
|
| }); | ||
|
|
||
| $('#app-navigation-toggle').click(function(){ | ||
| if($(window).width() > 768) { |
There was a problem hiding this comment.
remove this if, since the toggle should always be visible - then you won't need !important in CSS
There was a problem hiding this comment.
it for different behavior. on mobile devices app-content will be moved to the right. on desktop app-navigation will be hide.
There was a problem hiding this comment.
does it even work, @artemanufrij ? Look at the next line toggle('hidde') is not correct 😉
| @@ -62,7 +62,8 @@ | |||
| <?php print_unescaped($this->inc('part.navigation')); ?> | |||
| <?php /* print_unescaped($this->inc('part.settings')); */ ?> | |||
| </div> | |||
| <div id="app-content" ng-class="{ 'details-visible': sidebar.show }" ui-view> | |||
| <div id="app-content" ng-class="{ 'details-visible': sidebar.show }"> | |||
| <div ui-view></div> | |||
There was a problem hiding this comment.
why do you need additional <div> here?
There was a problem hiding this comment.
otherwise it doesn't work with core-functions
There was a problem hiding this comment.
which function exactly? I tested locally and didn't find any errors.
There was a problem hiding this comment.
the toggle button will be created by js.js from core (see line 1569) automatically. it works only if ui-view is in a separate div instead in app-content
There was a problem hiding this comment.
Why do we create a DOM element using JS if it has to be present in 100% of cases? Seems like a waste of resources. cc @juliushaertl
There was a problem hiding this comment.
@pixelipo This is part of the logic from core, so that apps don't need to care about adding this sidebar slide functionality. But since we use our own js code to toggle those of course we could also just keep the existing structure and add the toggle button to the template.
| <i class="icon icon-home"></i> | ||
| <span class="hidden-visually"><?php p($l->t('All Boards')); ?></span> | ||
| </a> | ||
| <div id="controls"> |
There was a problem hiding this comment.
please revert to previous version of this code. It was decided some time ago that it's a good approach. CHanges here are not needed to make toggle work.
I do agree with renaming #board-header to #controls
There was a problem hiding this comment.
no, but it's same code like files
There was a problem hiding this comment.
"same code as files" I remember that discussion from a different PR 😉 files also has a wrong. This is what is causing issue with positioning of icons mentioned by @juliushaertl
There was a problem hiding this comment.
I remember we discussed that some times ago, yes, although I'm not sure what exactly the reason was for that. 🙈 Let's use our fixed approach, we can always go to the way files does it once it is implemented properly there.
| <span class="hidden-visually"><?php p($l->t('All Boards')); ?></span> | ||
| </a> | ||
| <div id="controls"> | ||
| <div class="breadcrumb"> |
juliusknorr
force-pushed
the
toggle-navibar-on-mobile
branch
from
26f680b
to
2eddaab
Compare
css/style.css
Outdated
| @@ -173,7 +173,7 @@ button.button-inline:hover { | |||
|
|
|||
| #app-navigation-toggle { | |||
| width: 45px; | |||
There was a problem hiding this comment.
this should be 44 as well
js/app/Run.js
Outdated
| @@ -63,9 +63,9 @@ app.run(function ($document, $rootScope, $transitions, BoardService) { | |||
|
|
|||
| $('#app-navigation-toggle').click(function(){ | |||
| if($(window).width() > 768) { | |||
| $('#app-navigation').toggle('hidde'); | |||
| $('#app-navigation').toggle('hidden'); | |||
There was a problem hiding this comment.
👍 that's what I was talking about :)
juliusknorr
force-pushed
the
toggle-navibar-on-mobile
branch
from
2eddaab
to
db7a91e
Compare
Signed-off-by: Artem Anufrij <artem.anufrij@live.de>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
juliusknorr
force-pushed
the
toggle-navibar-on-mobile
branch
from
db7a91e
to
812ed56
Compare
Signed-off-by: Artem Anufrij artem.anufrij@live.de