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

SDK: refactor block CSS name-spacing #26662

Merged
merged 6 commits into from
Aug 15, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
4 changes: 4 additions & 0 deletions bin/sdk-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ yargs
description: 'Whether to watch for changes and automatically rebuild.',
type: 'boolean',
},
'namespace': {
description: 'Wheater CSS be namespaced.',
Copy link
Member

Choose a reason for hiding this comment

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

Typo here: should be whether. Also, I think it's missing a "should" right before "be".

type: 'boolean',
},
} ),
handler: argv => gutenberg.compile( argv )
} )
Expand Down
4 changes: 3 additions & 1 deletion bin/sdk/gutenberg.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ exports.compile = args => {
};

const name = path.basename( path.dirname( options.editorScript ).replace( /\/$/, '' ) );
const baseConfig = getBaseConfig( { extensionName: name } );
const baseConfig = getBaseConfig( {
extensionName: options.namespace ? name : false
Copy link
Member

Choose a reason for hiding this comment

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

With --namespace, this would result in:

.tiled-gallery .wp-block-a8c-tiled-gallery .tiled-gallery-square

and

.jetpack .wp-block-a8c-tiled-gallery .tiled-gallery-square

is that expected?

If so, wouldn't it be more robust if we have the namespace option as a string instead, which would allow us to specifically set the namespace as we use the SDK from the CLI?

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 hid the namespacer tool behind the flag instead of removing it because I think it might still be useful; I'm experimenting with it in #26683

Since it's broken like you describe, it should be considered only for development testing.

Do you reckon I should completely remove the namespacer in this PR even if we might add it back later on?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like @dmsnell is opting for removing it in #26680.

} );

const config = {
...baseConfig,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class JetpackMarkdownBlockEditor extends Component {
};

return (
<div>
<div className={ className }>
<BlockControls>
<ButtonGroup>
<button
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import MarkdownPreview from './components/markdown-renderer';

function JetpackMarkdownBlockSave( { attributes, className } ) {
return <MarkdownPreview className={ `${ className }-renderer` } source={ attributes.source } />;
return <MarkdownPreview className={ className } source={ attributes.source } />;
}

export default JetpackMarkdownBlockSave;
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ import { registerBlockType } from '@wordpress/blocks';
/**
* Internal dependencies
*/
import './markdown-editor.scss';
import JetpackMarkdownBlockEditor from './jetpack-markdown-block-editor';
import JetpackMarkdownBlockSave from './jetpack-markdown-block-save';

registerBlockType( 'jetpack/markdown-block', {
registerBlockType( 'a8c/markdown', {
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 also want to apply that to any other extensions? Publicize still uses jetpack-publicize, and we might want to be consistent with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Updated.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like this introduced a bug 😕

See #26726

title: __( 'Markdown' ),

description: [
Expand Down
8 changes: 8 additions & 0 deletions client/gutenberg/extensions/markdown/markdown-editor.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
.wp-block-a8c-markdown__placeholder {
opacity: 0.5;
pointer-events: none;
}
.wp-block-a8c-markdown__preview p {
min-height: 1.8em;
white-space: pre-wrap;
}
Copy link
Member Author

@simison simison Aug 13, 2018

Choose a reason for hiding this comment

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

I took this CSS from the original Jetpack-Markdown PR: https://github.com/Automattic/jetpack/pull/9705/files#diff-3e61a3fdc3d89ac00c7baef9a6d68d6c (kudos @Ferdev)

I left this bit out because I didn't find it being used anywhere:

.wp-block-jetpack-markdown-block__live-preview__token {
	/* $dark-gray-300 from Gutenberg _colors.scss */
	color: #6c7781;
}

...although it would've been a nice example of importing a Gutenberg variable in.

Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,10 @@ class TiledGalleryLayoutSquare extends Component {

render() {
const rows = this.computeItems();
const linkTo = this.props.linkTo;
const { linkTo, className } = this.props;

return (
<div className="tiled-gallery">
<div className={ className }>
<div className="tiled-gallery-square tiled-gallery-unresized" data-original-width={ CONTENT_WIDTH }>
{ rows.map( ( row, index ) => {
const styleAttr = {
Expand Down
5 changes: 3 additions & 2 deletions client/gutenberg/extensions/tiled-gallery/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,9 @@ class JetpackGalleryBlockEditor extends Component {
const imageTiles = (
<JetpackGalleryBlockSave
attributes={ {
images: images,
columns: columns,
className,
images,
columns,
linkTo: 'none',
} }
/>
Expand Down
5 changes: 2 additions & 3 deletions client/gutenberg/extensions/tiled-gallery/tiled-gallery.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@
import { __ } from '@wordpress/i18n';
import { createBlock, registerBlockType } from '@wordpress/blocks';

import './tiled-gallery.scss';

/**
* Internal dependencies
*/
import './tiled-gallery.scss';
import JetpackGalleryBlockEditor from './edit.js';
import JetpackGalleryBlockSave from './save.js';

const JetpackGalleryBlockType = 'jetpack/gallery';
const JetpackGalleryBlockType = 'a8c/tiled-gallery';

const settings = {
title: __( 'Tiled Gallery' ),
Expand Down
114 changes: 58 additions & 56 deletions client/gutenberg/extensions/tiled-gallery/tiled-gallery.scss
Original file line number Diff line number Diff line change
@@ -1,62 +1,64 @@
.tiled-gallery-square {
clear: both;
margin: 0 0 20px;
overflow: hidden;
}
.tiled-gallery-image {
margin: 2px !important; /* Ensure that this value isn't overridden by themes that give content images blanket margins */
}
.gallery-group {
float: left;
position: relative;
}
.gallery-row {
overflow: hidden;
}
.tiled-gallery-item {
float: left;
margin: 0;
position: relative;
width: inherit; /* prevents ie8 bug with inline width styles */
a { /* Needs to reset some properties for theme compatibility */
background: transparent;
border: none;
color: inherit;
margin: 0;
padding: 0;
text-decoration: none;
width: auto;
.wp-block-a8c-tiled-gallery {
.tiled-gallery-square {
clear: both;
margin: 0 0 20px;
overflow: hidden;
}
figure {
margin: 0;
.tiled-gallery-image {
margin: 2px !important; /* Ensure that this value isn't overridden by themes that give content images blanket margins */
}
.tiled-gallery-image,
.tiled-gallery-image:hover { /* Needs to reset some properties for theme compatibility */
background: none;
border: none;
box-shadow: none;
max-width: 100%;
padding: 0;
vertical-align: middle;
.gallery-group {
float: left;
position: relative;
}
.gallery-row {
overflow: hidden;
}
.tiled-gallery-item {
float: left;
margin: 0;
position: relative;
width: inherit; /* prevents ie8 bug with inline width styles */
a { /* Needs to reset some properties for theme compatibility */
background: transparent;
border: none;
color: inherit;
margin: 0;
padding: 0;
text-decoration: none;
width: auto;
}
figure {
margin: 0;
}
.tiled-gallery-image,
.tiled-gallery-image:hover { /* Needs to reset some properties for theme compatibility */
background: none;
border: none;
box-shadow: none;
max-width: 100%;
padding: 0;
vertical-align: middle;
}
}
}

.tiled-gallery-caption { /* Captions */
background: #eee;
background: rgba( 255,255,255,0.8 );
color: #333;
font-size: 13px;
font-weight: 400;
overflow: hidden;
padding: 10px 0;
position: absolute;
bottom: 0;
text-indent: 10px;
text-overflow: ellipsis;
width: 100%;
white-space: nowrap;
}
.tiled-gallery-caption { /* Captions */
background: #eee;
background: rgba( 255,255,255,0.8 );
color: #333;
font-size: 13px;
font-weight: 400;
overflow: hidden;
padding: 10px 0;
position: absolute;
bottom: 0;
text-indent: 10px;
text-overflow: ellipsis;
width: 100%;
white-space: nowrap;
}

.tiled-gallery-item-small .tiled-gallery-caption { /* Smaller captions */
font-size: 11px;
.tiled-gallery-item-small .tiled-gallery-caption { /* Smaller captions */
font-size: 11px;
}
}