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

toggle navibar visibility #177

Merged
merged 2 commits into from
Jun 9, 2017
Merged

toggle navibar visibility #177

merged 2 commits into from
Jun 9, 2017

Conversation

artemanufrij
Copy link
Member

Signed-off-by: Artem Anufrij artem.anufrij@live.de

Signed-off-by: Artem Anufrij <artem.anufrij@live.de>
@artemanufrij
Copy link
Member Author

fix for #123

@artemanufrij artemanufrij self-assigned this Jun 9, 2017
@artemanufrij artemanufrij added this to the 0.2.0 milestone Jun 9, 2017
@codecov
Copy link

codecov bot commented Jun 9, 2017

Codecov Report

Merging #177 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #177   +/-   ##
=======================================
  Coverage   76.12%   76.12%           
=======================================
  Files          33       33           
  Lines         955      955           
=======================================
  Hits          727      727           
  Misses        228      228

Signed-off-by: Artem Anufrij <artem.anufrij@live.de>
@artemanufrij artemanufrij requested a review from juliusknorr June 9, 2017 20:55
@pixelipo pixelipo self-requested a review June 9, 2017 21:34
@@ -902,6 +910,12 @@ button.button-inline:hover {
}
}

@media (max-width: 768px) {
#app-navigation-toggle {
display: none !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

@juliushaertl didn't you mention somewhere that navigation toggle should be available on mobile as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could dispense with navigation bar completely if we move all buttons/controls into board view.

Copy link
Member

Choose a reason for hiding this comment

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

Yes let's add it to mobile as well, since otherwise it is not possible to access archived boards there.

Copy link
Member

Choose a reason for hiding this comment

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

One more thing that needs to be fixed is that the button should also be visible on the boards list overview ;)

@@ -24,6 +24,10 @@ app.controller('AppController', function ($scope, $location, $http, $route, $log
$rootScope.sidebar = {
show: false
};
$rootScope.navibar = {
show: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance we could animate this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@pixelipo I have tried to animate it like files but I didn't found how it works in files.

It was my fist try :(

Copy link
Contributor

Choose a reason for hiding this comment

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

They have a different approach - They are using CSS to move (translate) the main container (right one) over the navigation pane :)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can just use the same mechanism as in files. 😉

Copy link
Contributor

@pixelipo pixelipo left a comment

Choose a reason for hiding this comment

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

I'm not sure my questions are out of the scope of this PR. Apart from them, everything works as expected.

App doesn't save "toggled" state, which might be issue for the case reported in #123

@artemanufrij artemanufrij merged commit b1ac1b3 into master Jun 9, 2017
@artemanufrij artemanufrij deleted the toggle-navi-bar branch June 9, 2017 21:57
@juliusknorr
Copy link
Member

App doesn't save "toggled" state, which might be issue for the case reported in #123

I guess that is ok for now, since it will stay hidden while using deck and just reappear on reload.

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.

3 participants