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.

Sign up for GitHub

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

Jump to bottom

Sidebar: Add a sidebar to hold the post settings and inspector #449

Merged
merged 3 commits into from
Apr 19, 2017
Merged

Sidebar: Add a sidebar to hold the post settings and inspector #449

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54c2fe9
Sidebar: Add a sidebar to hold the post settings and inspector
youknowriad Apr 19, 2017
c951824
Sidebar: Renaming the reducer and classnames
youknowriad Apr 19, 2017
09c00df
Rename isActive Prop
youknowriad Apr 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions editor/assets/stylesheets/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ $editor-font-size: 16px;
$editor-line-height: 1.8em;
$item-spacing: 10px;
$header-height: 56px;
$sidebar-width: 320px;
$admin-bar-height: 32px;
$admin-bar-height-big: 46px;
$admin-sidebar-width: 160px;
Expand Down
5 changes: 3 additions & 2 deletions editor/components/button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
import './style.scss';
import classnames from 'classnames';

function Button( { isPrimary, isLarge, className, ...additionalProps } ) {
function Button( { isPrimary, isLarge, isActive, className, ...additionalProps } ) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to rename the prop from isActive to isToggled as well?

Copy link
Contributor Author

@youknowriad youknowriad Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Like I said, "too fast and miss the details" :)

const classes = classnames( 'editor-button', className, {
button: ( isPrimary || isLarge ),
'button-primary': isPrimary,
'button-large': isLarge
'button-large': isLarge,
'is-active': isActive
} );

return (
Expand Down
6 changes: 6 additions & 0 deletions editor/components/button/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
.editor-button {
background: none;
border: none;
outline: none;

&:disabled {
opacity: 0.6;
}

&.is-active {
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect this to be the same styling we'll want for the :active pseudo-class ?

If so, should we add it here?
If not, should we be worried about confusion between the two unique states sharing the same name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know? :) I've always been confused with the :active state. I think .is-active and :active are different states, maybe we should rename .is-active?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a question for @jasmussen for prescribing variations on button state we'll need to support.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about hover, click and active states these days, across all controls, to ensure consistency. Right now the toggled state is visually the same as :active. Did that answer the question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means all the buttons will have the dark gray background when clicked (before releasing the mouse). I'm not sure we want this, do we?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, I'm tired and wasn't being very clear indeed.

My mind has been thinking about the hover state, which right now is blue text and a dark gray outline, which I'm not convinced about. I've been thinking whether we could do the inverted version for both hover, active and toggled. I.e. hover would also have the dark gray background. This is in context of the accessibility and contrast discussions that took place a while back. But we might find that this feels too heavy handed, and that we might be able to have a lighter gray hover+active state, in which case only the toggled state would be dark gray.

Bottomline, probably good to not hover/active/toggled states block this PR from being merged, as this is something I will revisit across all buttons later on regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm renaming is-active to is-toggled :)

background: $dark-gray-500;
color: $white;
}
}
10 changes: 6 additions & 4 deletions editor/header/tools/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import IconButton from '../../components/icon-button';
import Inserter from '../../components/inserter';
import Button from '../../components/button';

function Tools( { undo, redo, hasUndo, hasRedo } ) {
function Tools( { undo, redo, hasUndo, hasRedo, sidebarOpened, toggleSidebar } ) {
return (
<div className="editor-tools">
<IconButton
Expand All @@ -31,7 +31,7 @@ function Tools( { undo, redo, hasUndo, hasRedo } ) {
<Dashicon icon="visibility" />
{ wp.i18n._x( 'Preview', 'imperative verb' ) }
</Button>
<Button>
<Button onClick={ toggleSidebar } isActive={ sidebarOpened }>
<Dashicon icon="admin-generic" />
{ wp.i18n.__( 'Post Settings' ) }
</Button>
Expand All @@ -46,10 +46,12 @@ function Tools( { undo, redo, hasUndo, hasRedo } ) {
export default connect(
( state ) => ( {
hasUndo: state.blocks.history.past.length > 0,
hasRedo: state.blocks.history.future.length > 0
hasRedo: state.blocks.history.future.length > 0,
sidebarOpened: state.sidebar.opened,
} ),
( dispatch ) => ( {
undo: () => dispatch( { type: 'UNDO' } ),
redo: () => dispatch( { type: 'REDO' } )
redo: () => dispatch( { type: 'REDO' } ),
toggleSidebar: () => dispatch( { type: 'TOGGLE_SIDEBAR' } )
} )
)( Tools );
3 changes: 3 additions & 0 deletions editor/header/tools/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
color: $dark-gray-500;
margin-right: $item-spacing;
cursor: pointer;
height: 30px;
padding: 0 10px;
border-radius: 3px;

&:hover {
color: $blue-medium;
Expand Down
21 changes: 16 additions & 5 deletions editor/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,35 @@
* External dependencies
*/
import { connect } from 'react-redux';
import classnames from 'classnames';

/**
* Internal dependencies
*/
import './style.scss';
import Header from 'header';
import Sidebar from 'sidebar';
import TextEditor from 'modes/text-editor';
import VisualEditor from 'modes/visual-editor';

function Layout( { mode } ) {
function Layout( { mode, sidebarOpened } ) {
const className = classnames( 'editor-layout', {
'sidebar-opened': sidebarOpened
Copy link
Member

Choose a reason for hiding this comment

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

As a modifier class, I might expect some boolean-ish prefix to the class name, maybe has-open-sidebar ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or maybe is-sidebar-opened to match the reducer name

Copy link
Member

Choose a reason for hiding this comment

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

or maybe is-sidebar-opened to match the reducer name

Yep, that works.

} );

return (
<div>
<div className={ className }>
<Header />
{ mode === 'text' && <TextEditor /> }
{ mode === 'visual' && <VisualEditor /> }
<div className="editor-layout__content">
{ mode === 'text' && <TextEditor /> }
{ mode === 'visual' && <VisualEditor /> }
</div>
{ sidebarOpened && <Sidebar /> }
</div>
);
}

export default connect( ( state ) => ( {
mode: state.mode
mode: state.mode,
sidebarOpened: state.sidebar.opened
} ) )( Layout );
7 changes: 7 additions & 0 deletions editor/layout/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.editor-layout.sidebar-opened .editor-layout__content {
margin-right: $sidebar-width;

.editor-text-editor__formatting {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be nested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to be nested under .editor-layout.sidebar-opened, I can rewrite it :)

Copy link
Member

Choose a reason for hiding this comment

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

We could also define this in modes/text-editor/style.scss as assuming the existence of the ancestor modifier:

.editor-text-editor__formatting {
	// ...

	.editor-layout.is-sidebar-opened & {
		// ...
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just learned that we can do .editor-layout.is-sidebar-opened & And yes, this is probably better!

margin-right: $sidebar-width;
}
}
13 changes: 13 additions & 0 deletions editor/sidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* Internal Dependencies
*/
import './style.scss';

const Sidebar = () => {
return (
<div className="editor-sidebar">
</div>
);
};

export default Sidebar;
9 changes: 9 additions & 0 deletions editor/sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
.editor-sidebar {
position: fixed;
right: 0;
top: 32px + $header-height;
Copy link
Member

Choose a reason for hiding this comment

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

We'll need some breakpoint styling for this. Not sure if we wanted to do that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this for a more "designy" eyes :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll definitely tackle this separately, it's already on my to-do 😉

bottom: 0;
width: $sidebar-width;
border-left: 1px solid $light-gray-500;
background: $light-gray-300;
}
12 changes: 11 additions & 1 deletion editor/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,15 @@ export function mode( state = 'visual', action ) {
return state;
}

export function sidebar( state = { opened: false }, action ) {
Copy link
Member

Choose a reason for hiding this comment

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

What other properties do you expect this to hold? Wondering if a simpler isSidebarOpened reducer with a boolean value might be more appropriate for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hesitated, I was expecting we may have a property saying we're showing the PostSettings or the BlockInspector, but yeah. I'll go for isSidebarOpened right now!

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call it PostInspector when the time comes. The idea being that the entire sidebar is the inspector, but when nothing is selected, you are inspecting the post itself, therefore showing the meta boxes.

switch ( action.type ) {
case 'TOGGLE_SIDEBAR':
return { opened: ! state.opened };
}

return state;
}

/**
* Creates a new instance of a Redux store.
*
Expand All @@ -158,7 +167,8 @@ export function createReduxStore() {
blocks,
selectedBlock,
hoveredBlock,
mode
mode,
sidebar
} );

return createStore(
Expand Down
20 changes: 19 additions & 1 deletion editor/test/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
hoveredBlock,
selectedBlock,
mode,
sidebar,
createReduxStore
} from '../state';

Expand Down Expand Up @@ -281,6 +282,22 @@ describe( 'state', () => {
} );
} );

describe( 'sidebar()', () => {
it( 'should be closed by default', () => {
const state = sidebar( undefined, {} );

expect( state.opened ).to.be.false();
} );

it( 'should toggle the sidebar open flag', () => {
const state = sidebar( { opened: false }, {
type: 'TOGGLE_SIDEBAR'
} );

expect( state.opened ).to.be.true();
} );
} );

describe( 'createReduxStore()', () => {
it( 'should return a redux store', () => {
const store = createReduxStore();
Expand All @@ -297,7 +314,8 @@ describe( 'state', () => {
'blocks',
'selectedBlock',
'hoveredBlock',
'mode'
'mode',
'sidebar'
] );
} );
} );
Expand Down