Navbar refactor #883
Navbar refactor #883
Conversation
Justin & I cleaned the Navbar up and changed the styling a bit. Big internal refactor of the codebase.
app/addons/auth/base.js
Outdated
| } else if (session.isLoggedIn()) { | ||
| link = { | ||
| const link = { |
There was a problem hiding this comment.
This doesn't seem correct, the link variable is declared twice and then used outside its scope
| } | ||
|
|
||
| render () { | ||
| const mainClass = classNames( |
There was a problem hiding this comment.
Nice, this is very cool idea
| } | ||
|
|
||
| .faux-navbar__brand-logo--wide { | ||
| background: url(../../../../../assets/img/CouchDB-negative-logo.png) no-repeat 23px 0px; |
There was a problem hiding this comment.
It would be nice to set these logo's via our settings.json file. I know the pouchdb team would like to override these
| @@ -19,5 +19,9 @@ export default { | |||
| NAVBAR_SET_VERSION_INFO: 'NAVBAR_SET_VERSION_INFO', | |||
| NAVBAR_ACTIVE_LINK: 'NAVBAR_ACTIVE_LINK', | |||
| NAVBAR_HIDE: 'NAVBAR_HIDE', | |||
| NAVBAR_SHOW: 'NAVBAR_SHOW' | |||
| NAVBAR_SHOW: 'NAVBAR_SHOW', | |||
|
|
|||
There was a problem hiding this comment.
Minor, but no space is needed here
| createLinks (links) { | ||
| const { activeLink, isMinimized } = this.props; | ||
|
|
||
| return _.map(links, (link, i) => { |
There was a problem hiding this comment.
Can you rather do links.map. We don't need to use lodash for that.
| }, | ||
|
|
||
| setMenuState () { | ||
| FauxtonAPI.Events.trigger(FauxtonAPI.constants.EVENTS.NAVBAR_SIZE_CHANGED, this.state.isMinimized); |
There was a problem hiding this comment.
Is this trigger event still needed. Could we rather use a store instead. I would like to remove all our trigger events as much as possible.
| @@ -94,8 +107,7 @@ Stores.NavBarStore = FauxtonAPI.Store.extend({ | |||
| }, | |||
|
|
|||
| toggleMenu () { | |||
| app.utils.localStorageSet(FauxtonAPI.constants.LOCAL_STORAGE.SIDEBAR_MINIMIZED, | |||
| !this.isMinimized()); | |||
| this._isMinimized = !this._isMinimized; | |||
There was a problem hiding this comment.
This means we are no longer keeping state of what the user preferred in terms of minimized or open navbar?
There was a problem hiding this comment.
Looks that way, yeah. @justin-mcdavid-ibm: did you discuss this with @robertkowalski during the initial design phase for this?
There was a problem hiding this comment.
We made the decision to open the dashboard with main-nav collapsed, and a clearer trigger for expanding it back out. I don't think preferences need to be maintained across sessions, especially if the default state is the one providing more screen real estate.
These decisions came out of user feedback regarding confusion on the "hamburger" icon's functionality, and requests for more visual space to work with.
There was a problem hiding this comment.
I think I'd prefer to leave this as-is. The new logic forces the sidebar to be minimized by default, showing users that they have much more screen real estate to work with, which is inline with the user research and feedback.
|
|
||
| import _ from 'lodash'; |
There was a problem hiding this comment.
I don't think this is needed.
|
I'm happy with that
All misspelling thanks to my iPhone.
________________________________
From: Ryan Millay <notifications@github.com>
Sent: Thursday, March 30, 2017 10:41:00 PM
To: apache/couchdb-fauxton
Cc: garren smith; Comment
Subject: Re: [apache/couchdb-fauxton] Navbar refactor (#883)
@millayr commented on this pull request.
________________________________
In app/addons/fauxton/navigation/stores.js<#883 (comment)>:
@@ -94,8 +107,7 @@ Stores.NavBarStore = FauxtonAPI.Store.extend({
},
toggleMenu () {
- app.utils.localStorageSet(FauxtonAPI.constants.LOCAL_STORAGE.SIDEBAR_MINIMIZED,
- !this.isMinimized());
+ this._isMinimized = !this._isMinimized;
I think I'd prefer to leave this as-is. The new logic forces the sidebar to be minimized by default, showing users that they have much more screen real estate to work with, which is inline with the user research and feedback.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#883 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAK9AkXB8ALnpNUsstRKQbsywKmcNWZHks5rrBNcgaJpZM4Mm_i->.
|
| @@ -47,7 +51,9 @@ | |||
| "dest": "dist/debug/index.html", | |||
| "variables": { | |||
| "title": "Project Fauxton", | |||
| "generationLabel": "Fauxton Couchapp" | |||
| "generationLabel": "Fauxton Couchapp", | |||
|
This is coming along nicely. I'm seeing two issues.
Then if I make my screen narrow. The navbar struggles a bit: |
| <div className={navClasses}> | ||
| <nav> | ||
| <Burger isMinimized={isMinimized} toggleMenu={this.toggleMenu}/> | ||
| <div className="faux-navbar__flex"> |
There was a problem hiding this comment.
This is really minor. But the name faux-navbar__flex doesn't feel right. I think something more to describe what it does in relation to the navbar would make more sense.
There was a problem hiding this comment.
no worries. I renamed it to faux-navbar__linkcontainer
Trying to take over #826