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

Try inserter with collapsable panels #6636

Merged
merged 12 commits into from May 23, 2018

Conversation

Projects
None yet
7 participants
@youknowriad
Contributor

youknowriad commented May 8, 2018

screen shot 2018-05-08 at 10 33 17

related #6168

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented May 8, 2018

Holy cow, this is just so so so so so good. This feels like a dramatic improvement over what was there before. The border on hover feels like a good compromise between the pill icons and the original design (and means plugin developers won't need to provide a hover state, which is keeps it simple).

@shaunandrews

This comment has been minimized.

shaunandrews commented May 8, 2018

Any thoughts on simplifying the hover? Maybe take a cue from the toolbar hover:

screen shot 2018-05-08 at 11 56 57 am

--

Can we make the icons bigger (while trying to keep the buttons the same size) and darker?

--

Any thoughts on reducing the number of blocks we show in the "Most Used" group? Three rows fill the entire height of the popover, making it harder to notice that it scrolls (at least, on OS X).

screen shot 2018-05-08 at 11 59 23 am

Alternatively, we could make the height of the popover taller by default to fit three rows, and a bit of the next accordion.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 9, 2018

I tweaked the icons to make them bigger darker, changed the hover style and added a light box-shadow we used to use to make it more obvious that scrolling is possible.

@youknowriad youknowriad requested a review from karmatosed May 9, 2018

@karmatosed

This comment has been minimized.

Member

karmatosed commented May 9, 2018

I have a few thoughts based on the PR right now. I am cautious to give a design review until we have a few things smoothed.

Overall, I like it, there are however a few feelings I can't shake using this. The first is of this visually being really closed in now. There is a density to the interface that wasn't there before and I think it's not better off for that. I didn't get that feeling in the mocks as much as using it I do. It also is incredibly hard to read and connect the titles with the icons. That all said, I feel it's adjustments we need over anything drastic, which is really awesome.

This all said, it's far better as a foundation than we have now, let's iterate!

disconnect

Looking at above there are a few things stand out as things we need to work on:

  • The hover is almost un-noticeable.
  • The hover and searching being in focus feels because of similarity visually disorientating.
  • There is a visual density I'm not sure about and which makes it very hard to now read the small text under icons.
  • I find the icons too dominant at this size. Maybe we can iterate and find a middle ground?
  • I feel like the icons are disconnected too much, the hover impacts this by not including text. I do not know if the answer is including text as much as easing that disconnect.
  • The contrast between the background grey and icon makes the design feel super close. Could we try opening up with a lighter grey?

I would agree, let's reduce the number shown in the most used section.

( [ edit ] accidentally closed because of the placing of buttons... it's so annoying. )

@karmatosed karmatosed closed this May 9, 2018

@karmatosed karmatosed reopened this May 9, 2018

@shaunandrews

This comment has been minimized.

shaunandrews commented May 10, 2018

inserter hovers

I made some tweaks but I can't commit them for some reason — git keeps telling me I don't have permission to commit. Here's the diff to get to the GIF above:

diff --git a/editor/components/inserter/style.scss b/editor/components/inserter/style.scss
index 24f1c06d..e50a5116 100644
--- a/editor/components/inserter/style.scss
+++ b/editor/components/inserter/style.scss
@@ -87,20 +87,23 @@ input[type="search"].editor-inserter__search {
 .editor-inserter__item {
 	display: inline-flex;
 	flex-direction: column;
-	width: calc( 33% - 2px );
-	margin: 1px;
+	width: calc( 33.3% - 4px );
+	margin: 0 2px 8px 2px;//1px;
 	font-size: $default-font-size;
 	color: $dark-gray-500;
-	padding: 8px 6px;
+	padding: 0;//8px 6px;
 	align-items: stretch;
 	justify-content: center;
 	cursor: pointer;
 	border: none;
-	line-height: 1em;
+	//line-height: 1em;
 	background: transparent;
-	min-height: 5em;
+	//min-height: 5em;
 	overflow: hidden;
 	word-break: break-word;
+	border-radius: 3px;
+	border: 1px solid transparent;
+	transition: all 0.03s ease-in-out;
 
 	&:disabled {
 		@include button-style__disabled;
@@ -108,8 +111,18 @@ input[type="search"].editor-inserter__search {
 
 	&:not(:disabled) {
 		&:hover {
+			//background: $light-gray-200;
+			border-color: $light-gray-300;
+			box-shadow: 0 1px 0 $light-gray-300;
+			transform: scale( 1.1 );
+
 			.editor-inserter__item-icon {
-				border: 1px solid $dark-gray-800;
+				//background: $light-gray-400;
+				color: $dark-gray-800;
+
+				svg {
+					//transform: scale( 1.1 );
+				}
 			}
 
 			.editor-inserter__item-title {
@@ -128,14 +141,18 @@ input[type="search"].editor-inserter__search {
 
 .editor-inserter__item-icon {
 	padding: 7px 20px;
-	background: $light-gray-400;
-	border-radius: 3px;
-	margin-bottom: 5px;
-	color: $dark-gray-800;
-	border: 1px solid transparent;
+	background: $light-gray-200;
+	border-radius: 3px 3px 0 0;
+	color: $dark-gray-500;
+	transition: all 0.05s ease-in-out;
 
 	svg {
-		width: 25px;
-		height: 25px;
+		width: 22px;
+		height: 22px;
+		transition: all 0.2s ease-out;
 	}
 }
+
+.editor-inserter__item-title {
+	padding: 4px 2px;
+}

@jayshenk jayshenk referenced this pull request May 10, 2018

Closed

Add Inline Images and Inline Blocks API #5794

4 of 4 tasks complete
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 11, 2018

Thanks @shaunandrews for the tweaks, I pushed the changes.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 11, 2018

After some time using it, I think I personally I don't like the panels :(. I do like the simplification this brings, but I honestly think it adds too many clicks and scrolls to access the desired block.

I'd prefer just a single list of blocks with titles to separate different sections, simple and efficient where you see all blocks with a scrollbar. I feel the tabs or panels are not necessary at all and just add visual density.

Something like the emoji selector in MacOS. They do have something like tabs at the bottom where the visible section is highlighted while you scroll but I don't it's necessary for us. They have something interesting if you want a more complete UI, the button at the top right opens a bigger modal where the sections are more organized. I feel it's a good addition for advanced filtering if needed but not something mandatory or that we should try to bake into the small inserter popover.

So yes, maybe instead of having one popover with a lot of information there, we could instead build a simple list on the inserter with a button to open a richer UI to navigate the blocks. (the richer UI is optional)

Simple popover:

screen shot 2018-05-11 at 10 40 05

More advanced modal:

screen shot 2018-05-11 at 10 43 37

@karmatosed

This comment has been minimized.

Member

karmatosed commented May 11, 2018

Whilst it is an extra click I think there is merit in this format. I have to be honest @youknowriad, I think having 2 formats as the emoji picker is adding complication, that said I'd need to see a design to be sure. It works for emoji's because of their nature and I am not sure block selecting and emoji picking is the same mindset. At the root perhaps it is 'picking' but there's a few more levels of process going on with expressing in emoji and adding content. I also have concerns over usability in a giant list.

What we have now I think could have some iterating. I actually think the 'fixed' bottom accordions could be worth exploring again. As we hide them below in the list it's causing a short cut to be lost. I realise the scaling of this is limiting but maybe having a larger library isn't the worst thing.

The new hovering I don't see in the latest branch, but that could be because we have failing tests maybe? Looking at the gif @shaunandrews provides I absolutely love it and feel it really meets all the concerns I had previously.

This all said, I feel we've come a long way so far and what we have I wouldn't be at all unhappy with adding in as is. Why? Well, right now I feel this is better than what we currently have. Absolutely we should polish this, but we already have such a better block picking experience in this version.

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented May 11, 2018

Agreed with @karmatosed, 100%. I think this is a dramatic improvement on the UX. There are definitely further improvements possible, but this is such a fantastic step in the right direction.

A few small riffs on this:

I think when you hover, the elements kind of blend together because they overlap and the grey runs into each other. A second box-shadow — essentially a fake white border — might help give some separation between elements on hover:

kapture 2018-05-11 at 8 54 53

// Tweak at inserter/style.css, line 116
box-shadow: 0 1px 0 $light-gray-300, 0 0 0 2px white;

Also, it's slightly nit-picky and probably just me (I know it's been this way for a while), but the outline on the search input cutting off the arrow feels kinda clunky to me. What if the outline is positioned inside a bit?

untitled_5_sketch

This is outline-offset at -5px.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented May 14, 2018

I think this can work!

I think if we go this route, we should default to a taller inserter, obviously keeping the viewport in mind. I also think the icon inside the gray swatch should be darker to increase contrast.

Right now the shadow is applied to the entire link. I can't find where @shaunandrews's original hover mockup is — but seems like we should have both a thicker drop shadow (the block scales up, so it implies a fair bit of elevation), but also be mindful of what that shadow applies to. Right now it looks like the gray swatch is the "card", and the text floats in the air, right until you over it and realize the white space below the text is also part of the card. We want to make sure to treat the "materials" right, when we're using shadows here.

@gravityrail

This comment has been minimized.

Contributor

gravityrail commented May 18, 2018

@chrisvanpatten I implemented some of your changes:

  • increase dialog height to 350px
  • darker border for hover state
  • reduce outline-offset for search dialog

cc @shaunandrews

Screenshot:

block-with-hover

@gravityrail

This comment has been minimized.

Contributor

gravityrail commented May 18, 2018

I also just added a quick change to add per-block class names in the item list, following the same pattern as the class names that wrap blocks in the editor.

This allows much simpler branding/customisation of the blocks:

e.g.

/* icons for the Gutenberg editor */
.editor-block-list-item-jetpack-vr  {
	position: relative;
	.editor-inserter__item-icon {
		background-color: #00BE28;
		color: white;
		&:hover {
			color: white;
		}
		&::before {
			font-family: 'jetpack';
			font-size: 20px;
			content: '\f100';
			color: white;
			position: absolute;
			top: 9px;
			right: 15px;
		}
	}
}

produces:

jetpack-icon

@gravityrail

This comment has been minimized.

Contributor

gravityrail commented May 18, 2018

Noting here for drive-by reviewers that @youknowriad intends to fix these tests some time on Monday. I'm not yet fully up to speed in Gutenberg-land. Or Javascript in general.

@karmatosed

This comment has been minimized.

Member

karmatosed commented May 20, 2018

Really great to see the insights and thinking here.

@jasmussen:

I think if we go this route, we should default to a taller inserter, obviously keeping the viewport in mind. I also think the icon inside the gray swatch should be darker to increase contrast.

I agree. I think we have space and should use it a little. Not too tall but taller :)

@gravityrail thanks for diving into work here and look forward to giving your PR a test.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 21, 2018

Fix the unit tests and applied some tweaks. What's remaining here?

@mtias

This comment has been minimized.

Contributor

mtias commented May 21, 2018

This is looking good to me.

@shaunandrews @karmatosed @jasmussen I think the "shared" panel should be at the bottom. And can we add the icon we use for shared blocks to the panel to make the visual association?

The hover still has blurry shadows instead of simpler borders:

https://user-images.githubusercontent.com/191598/39887780-0ef59d82-5461-11e8-9640-467d4202bc28.gif

@mtias

This comment has been minimized.

Contributor

mtias commented May 21, 2018

Also, what do you think of not treating the first group as a named category and making them always visible?

@gravityrail

This comment has been minimized.

Contributor

gravityrail commented May 21, 2018

I will add the simpler borders @mtias

@shaunandrews

This comment has been minimized.

shaunandrews commented May 21, 2018

Made the icons darker, simplified the border (though I'm unclear what the "right" hover effect is here...) and made the columns wider. I also changed the experimental label for columns to beta — this avoids an awkward text wrap, but maybe beta isn't the right word?

diff --git a/core-blocks/columns/index.js b/core-blocks/columns/index.js
index fe7f6fed..c54d8282 100644
--- a/core-blocks/columns/index.js
+++ b/core-blocks/columns/index.js
@@ -44,7 +44,7 @@ export const settings = {
 		/* translators: Block title modifier */
 		__( '%1$s (%2$s)' ),
 		__( 'Columns' ),
-		__( 'Experimental' )
+		__( 'beta' )
 	),
 
 	icon: 'columns',
diff --git a/editor/components/inserter/style.scss b/editor/components/inserter/style.scss
index 93c3c48d..b0947781 100644
--- a/editor/components/inserter/style.scss
+++ b/editor/components/inserter/style.scss
@@ -86,16 +86,16 @@ input[type="search"].editor-inserter__search {
 }
 
 .editor-inserter__item-list {
-	margin: 0 -10px;
+	margin: 0 -8px;
 }
 
 .editor-inserter__item {
 	display: inline-flex;
 	flex-direction: column;
-	width: calc( 33.3% - 20px );
-	margin: 0 10px 8px 10px;
+	width: calc( 33.3% - 8px );
+	margin: 0 4px 8px 4px;
 	font-size: $default-font-size;
-	color: $dark-gray-500;
+	color: $dark-gray-700;
 	padding: 0;
 	align-items: stretch;
 	justify-content: center;
@@ -114,12 +114,12 @@ input[type="search"].editor-inserter__search {
 
 	&:not(:disabled) {
 		&:hover {
-			border-color: $light-gray-300;
-			box-shadow: 0 1px 0 1px $light-gray-500, 0 0 0 2px white;
-			transform: scale( 1.1 );
+			border-color: $light-gray-600;
+			box-shadow: 0 0 0 2px white;
+			transform: scale( 1.15 );
 
 			.editor-inserter__item-icon {
-				color: $dark-gray-800;
+				color: $dark-gray-900;
 				border-radius: 3px 3px 0 0;
 			}
 
@@ -142,7 +142,7 @@ input[type="search"].editor-inserter__search {
 	padding: 7px 20px;
 	background: $light-gray-200;
 	border-radius: 3px;
-	color: $dark-gray-500;
+	color: $dark-gray-700;
 	transition: all 0.05s ease-in-out;
 
 	svg {
@gravityrail

This comment has been minimized.

Contributor

gravityrail commented May 21, 2018

I just applied most of @shaunandrews' changes, reviewing with him now in Slack

@gravityrail

This comment has been minimized.

Contributor

gravityrail commented May 21, 2018

@mtias @shaunandrews @youknowriad @karmatosed pushed up changes, what do you think?

latest-inserter-menu-style

@gravityrail

This comment has been minimized.

Contributor

gravityrail commented May 21, 2018

Ditched zoom for now, was hard to get it to look right and not touch/overlap button focus border etc

ditched-the-zoom

@gravityrail

This comment has been minimized.

Contributor

gravityrail commented May 21, 2018

@chrisvanpatten we tried that but it caused issues with overlapping the bottom border of the accordion button.

@karmatosed

This comment has been minimized.

Member

karmatosed commented May 22, 2018

I am actually glad zoom isn't added, I have worries about that as a pattern being brought into this interface.

I got a lot of conflicts trying to view this PR, can anyone else replicate.

@mtias mtias requested a review from WordPress/gutenberg-core May 23, 2018

@mtias

mtias approved these changes May 23, 2018

Let's try this.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 23, 2018

Thanks all for your work on this one

@youknowriad youknowriad merged commit 3fe3868 into master May 23, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.13% but relative coverage increased by +26.72% compared to 76e7b2a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/inserter-v3 branch May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment