Close side menu when pressing settings #8
Conversation
| usersView: state.ui.scenes.controlPanel.usersView | ||
| }) | ||
| const mapDispatchToProps = (dispatch) => ({ | ||
| closeSideMenu: () => dispatch(closeSideMenu()), |
There was a problem hiding this comment.
Is there a documented coding convention that the prop parameter be the same name as the action creator? This makes it very difficult to surf code and make refactoring changes. If this not a convention, then I'd ask we use closeSideMenu: () => dispatch(dispatchCloseSideMenu()),
paullinator
left a comment
paullinator
left a comment
There was a problem hiding this comment.
All good with just request that dispatch function names be different than props parameters.
| export const LIST_USERS_SIDEBAR = 'LIST_USER_USER_SIDEBAR' | ||
| export const SELECT_USERS_SIDEBAR = 'SELECT_USERS_SIDEBAR' | ||
| export const REMOVE_USERS_SIDEBAR = 'REMOVE_USERS_SIDEBAR' | ||
| export const LIST_USERS_SIDE_MENU = 'LIST_USER_USER_SIDE_MENU' |
There was a problem hiding this comment.
Is the word USER supposed to be in here twice?
| brandDanger: '#d9534f', | ||
| brandWarning: '#f0ad4e', | ||
| brandSidebar: '#252932', | ||
| brandSideMenu: '#252932', |
There was a problem hiding this comment.
Just a sidenote, at some point maybe we should have all theme colors in one file. I added them to themes/variables/airbitz.js, and it would be best to not have the color hex values defined in more than one place.
There was a problem hiding this comment.
I like to create a constants file for items like colors.
you can see an example in core-ui-js.
then I do
import * as Colors from '../constants/Colors.js'
or
import * as Colors from '../constants/'
since the colors are in the overall index.js.
variables is a misleading title for items that are not changing.
There was a problem hiding this comment.
I fully agree. This is a good time to just make the color palette file and use it now. Please make the change to the pull request
There was a problem hiding this comment.
Realizing that this change didn't introduce these colors, I retract my previous comment. Let's put the colors in a Colors file in a different commit.
Close side menu when pressing settings.
Rename sidebar -> sideMenu