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

Add a minimal multi-selection block panel #12050

Merged
merged 5 commits into from
Nov 19, 2018

Conversation

youknowriad
Copy link
Contributor

Let's avoid the "coming soon" message in the RC and replace the multi-selection inspector panel with a minimal panel with some numbers.

screen shot 2018-11-19 at 10 44 45

@youknowriad youknowriad added this to the 4.5 milestone Nov 19, 2018
@youknowriad youknowriad self-assigned this Nov 19, 2018
@youknowriad youknowriad requested review from jasmussen and a team November 19, 2018 09:46
@gziolo
Copy link
Member

gziolo commented Nov 19, 2018

Code wise, it looks good. My concern is that we repeat 3 blocks in the tab's title and the body of the tab.

@gziolo gziolo added [Priority] High Used to indicate top priority items that need quick attention [Feature] Blocks Overall functionality of blocks labels Nov 19, 2018
@jasmussen
Copy link
Contributor

Let me try and push a little change.

@jasmussen
Copy link
Contributor

Okay, I pushed a little change:

screenshot 2018-11-19 at 11 11 30

This makes it look more like the normal inspector card:

screenshot 2018-11-19 at 11 12 16

I agree, when seeing this, with Gzregorz — can we make it so the "Block" tab title never changes? I.e. it should be this:

screenshot 2018-11-19 at 11 12 57

@youknowriad
Copy link
Contributor Author

I love it 👍


function MultiSelectionInspector( { blocks } ) {
return (
<div className="editor-block-inspector__card">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of reusing a className, I'd just duplicate this style in a style.scss file specific to this component and rename this class editor-multi-selection-inspector__card.

Later, we should think at a generic card component or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other classNames should also be renamed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other classNames should also be renamed.

Which ones? Did I miss any?

@youknowriad
Copy link
Contributor Author

Thanks for the tweaks 👍

@jasmussen
Copy link
Contributor

Forgot one, that I just pushed.

I'll need help changing the tab label, though — so it always says "Block".

@jasmussen
Copy link
Contributor

Okay I fixed the block tab:

screenshot 2018-11-19 at 11 28 02

I think this is good.

@youknowriad
Copy link
Contributor Author

There's still some things we can remove :) I'm pushing a commit

@jasmussen
Copy link
Contributor

jasmussen commented Nov 19, 2018

Thanks, Riad. I'm done with my changes. You can take it from here.

Here's why I think the block tab change is good, just for posterity:

multiselect

It's less jumpy, simpler, more consistent.

@youknowriad youknowriad force-pushed the add/mimimal-multiselection-inspector branch from 1a34599 to edfd969 Compare November 19, 2018 10:37
@youknowriad
Copy link
Contributor Author

I tweaked the last commit, we just need a ✅ now :)

@jasmussen
Copy link
Contributor

@gziolo, you're up! :D

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for addressing my comment and adding more visual tweaks. I like that we use this icon to look more similar to the regular block 👍

@gziolo gziolo merged commit b198e5c into master Nov 19, 2018
@gziolo gziolo deleted the add/mimimal-multiselection-inspector branch November 19, 2018 11:21
{ sprintf( __( '%d blocks' ), blocks.length ) }
</div>
<div className="editor-multi-selection-inspector__card-description">
{ sprintf( __( '%d words.' ), wordCount( serialize( blocks ), 'words' ) ) }
Copy link
Member

Choose a reason for hiding this comment

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

This needs to use _n() instead of __(). Also, there is no translators comment.

} showColors />
<div className="editor-multi-selection-inspector__card-content">
<div className="editor-multi-selection-inspector__card-title">
{ sprintf( __( '%d blocks' ), blocks.length ) }
Copy link
Member

Choose a reason for hiding this comment

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

This needs to use _n() instead of __(). Also, there is no translators comment.

swissspidy added a commit that referenced this pull request Nov 19, 2018
@swissspidy
Copy link
Member

Created #12054 to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants