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

Try G2 #19344

Open
wants to merge 44 commits into
base: master
from
Open

Try G2 #19344

wants to merge 44 commits into from

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 27, 2019

closes #18195
closes #18069
closes #18223
closes #16617
closes #19624
closes #7646
closes #19018
closes #12260
closes #15489

This branch should be used to serve as a base branch for all updates related to this issue #18667 that can't land individually.

For now, it contains:

  • Movers moved to the toolbar #18685
  • Change the block toolbar borders to black.
  • Simplify hover/focus and radius styles for Buttons.
  • Remove is-selected styles and replace with focus styles.
  • white placeholders with dotted background.
  • Block Switcher with an arrow on the corder of block toggle.
  • New Dropdown Menu styles.
  • No arrow indicator for dropdowns.
  • Update the icons used.
@youknowriad youknowriad force-pushed the try/g2 branch 2 times, most recently from be6191b to e93a5cd Dec 27, 2019
@youknowriad youknowriad mentioned this pull request Dec 27, 2019
1 of 3 tasks complete
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Jan 6, 2020

I am just exploring this a little and I feel the dots on the placeholders are a bit much with text. I personally find it hard to read. I wonder if a balance could be done to make them less strong @jasmussen?

g2visualissues

For me, there is an almost wave visual created that doesn't help text read clearly.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Jan 6, 2020

Another issue is with screens where multiple white backgrounds show and a theme isn't white, this feels a lot and like the focus hierarchy is a little misaligned:

toomanybackgrounds

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 6, 2020

I would tend to agree specifically with the dots as they are. They are meant to indicate the presence of a grid which blocks align themselves to. This is probably an aspect of the design that is worth postponing implementing until we can actually support a grid for the blocks.

I'm catching up on a few things today, but will give this PR a spin as soon as I can, perhaps help push some polish as well!

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 7, 2020

Thanks so much for taking a stab at this, Riad! I'm very excited to see the new UI explored, and I've been wondering how to best do it. I'm increasingly leaning towards a big switch, like this, rather than implementing things piecemeal.

I took this for a quick spin today, and it's exciting to explore as a prototype currently and as something that can grow into the real thing. There are a lot of little things I can help fix in code — the right black, the right border radius, the right spacing for icons, the right menu treatment, the right placeholder treatment, right toolbar positioning — mostly a massaging of pixels. But it's also likely going to be a great deal of CSS and code refactoring (touching a lot of files), so I'm currently pondering how to best help, rather than just jumping in and pushing code.

To be sure, this is the right branch to work in, correct? Or should I work in your "g3" branch instead?

One thing that does seem worth exploring separately, is any conversations around icons and fonts, which are tentatively explored also in the #18667 ticket.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 7, 2020

The reason for g2 and g3 branches is that I think we can probably land half of the changes (all the changes within the canvas) before the other half (the panels, the bordered dropdowns).

So if it's something in the canvas itself, push to this PR (and potentially rebase g3 on top of it) and if it's outside the canvas, it's the other one.

That said, if you don't think that split make sense, we can work on a single branch for everything. I personally do like 2 branches because the whole thing can become huge and instead of one giant PR, if we can do two bigs PRs that's a win.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 7, 2020

Cool, I'll think for a bit.

I'm also thinking in terms of introducing some grid/dimension variables and the new black color in a way that is leveraged by both.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 8, 2020

I'll be pushing some work today. Right at this moment I'm going to try the following approach:

  • try/g2, I'll focus on in-canvas work, but I will probably also be including popovers, as they appear prominently inside the canvas
  • try/g3, when I get to it, is outside, including modals

Icons and font discussions we postpone, I would suggest.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 8, 2020

Alright, I pushed a bunch of polish.

The placeholder grid is gone for now, and it has a 2px border radius to match the mockups:

Screenshot 2020-01-08 at 13 11 22

It has a focus style that is the beginning of a unitified theme-color 2px focus style we can use everywhere:

Screenshot 2020-01-08 at 13 11 42

I added new grid system variables, and cleaned up the code that paints borders around buttons, so they're all square:

Screenshot 2020-01-08 at 13 11 34

Note how the formatting buttons are also optically spaced:

Screenshot 2020-01-08 at 13 10 13

That is — when multiple buttons are shown next to each other, the middle ones are 36px, and the ones on the edges are 36px + 6px padding. This is based on the overlap the 48px buttons would have, in order to be optically balanced:

Screenshot 2020-01-08 at 12 56 22

There are a number of issues remaining, which I will help take a stab at. However before jumping into that, @youknowriad (get well soon!) I hope you can help me with a good rebase on this one. I know that @ellatrix has refactored the canvas a fair bit, removing dom nodes left and right, so before I go styling elements that no longer exist, it would be good to rebase. If you feel like squashing a couple of your smaller commits too, that would be fine ;)

Some of the remaining issues I hope to look at include:

Problem with focus on floats and other non-text blocks

Screenshot 2020-01-08 at 13 11 38

Problem with the toolbar needing to be detached so it can align left and right

Screenshot 2020-01-08 at 13 19 12

I don't know if that should be this branch, but the whole G2 concept of a detached toolbar kind of hinges upon it. Especially since we're still figuring out the exact behavior if the mover control; it's actually the block switcher that's supposed to align with the left margin on the block.

There's an extra border on the first block when you're multi selecting

Screenshot 2020-01-08 at 13 11 55

Also:

  • buttons inside toolbar needs consistent focus and and toggled styles
  • movers need polish
  • navigation mode needs redoing just like the rest
  • we need to refactor away the block-edit::before border
  • test plugin toolbar groups and plugins that add to the inline slot

Again, fonts and icons, and actually colors also, we'll look at separately.

@@ -4,10 +4,16 @@
width: 100%;
overflow: auto; // Allow horizontal scrolling on mobile.
position: relative;
border-left: $border-width solid $border-color;
height: $block-toolbar-height;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 8, 2020

Author Contributor

Does this work in all breakpoints and top toolbar, I think this generic component is meant to be shown with different heights, so maybe it should be unopionated?

This comment has been minimized.

Copy link
@jasmussen

jasmussen Jan 10, 2020

Contributor

Done!

$block-controls-height: 36px;
$icon-button-size: 36px;
$block-controls-height: 36px; // @todo: This should be deprecated in favor of 48px block control heights.
$icon-button-size: 36px; // @todo: This should be deprecated in favor of 48px block control heights.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 8, 2020

Author Contributor

This I think is useful for the default button size (and all what should have its size)

This comment has been minimized.

Copy link
@jasmussen

jasmussen Jan 10, 2020

Contributor

Can you elaborate on what you mean here?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 10, 2020

Author Contributor

I mean I don't really understand why this is deprecated. The button size is still 36px across the UI (aside toolbars)

This comment has been minimized.

Copy link
@jasmussen

jasmussen Jan 10, 2020

Contributor

Oh, good point. I suppose I'll remove those Todos for now. I think there is some variable cleanup todo here so that there isn't any confusion — "icon button" could refer to formatting buttons which should be 48x48, also on mobile, and perhaps even in the toolbar, wheras yes, the traditional blue primary and secondary buttons are 36 in height.

I'm speaking specifically about the clickable hit area also.

// They are 36px, except for the first and the last, which have
// an additional 6px padding left or right.
.components-toolbar > div {
width: 48px - 6px - 6px;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 8, 2020

Author Contributor

I wonder if it's possible for third-party plugins to add formatting buttons that are not icons but text (so not square).
Can we achieve the same behavior without forcing a width?

This comment has been minimized.

Copy link
@jasmussen

jasmussen Jan 10, 2020

Contributor

Addressed this in 0417c92, but it's a little messy and specific. Advice is welcome, but I'm also hoping to revisit this.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 8, 2020

Great thoughts Riad, I will try to address those tomorrow as I continue with this.

When you're well again, I would appreciate your help with a rebase!

@youknowriad youknowriad force-pushed the try/g2 branch from 1260380 to 651e9ff Jan 9, 2020
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 9, 2020

The rebase was a little bit painful :) I hope I didn't break anything.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 9, 2020

Polished the mover icons:

Screenshot 2020-01-09 at 12 03 52

Screenshot 2020-01-09 at 12 06 31

The hit area remains 48x24 pixels, despite the alignment of the icons inside, so the hit area remains a fair bit larger than what is currently shipping (28x24px).

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 9, 2020

Screenshot 2020-01-09 at 12 15 42

This white square at the end of the toolbar appeared in the rebase. This is from the redone and way faster sibling inserter, it exists there for tabbing but is invisible until focused. I will see what I can do to fix it so it doesn't take up space like it does now.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 9, 2020

Fixed the blank square:

Screenshot 2020-01-09 at 12 33 17

&.is-visible {
width: auto;
opacity: 1;
pointer-events: all;
}
}

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 9, 2020

Author Contributor

@ellatrix I feel the inserter shouldn't be part of the BlockToolbar DOM node at all, it makes styling more difficult. Could we return maybe a fragment and move it out?

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jan 9, 2020

Member

Sure. I moved it here temporarily. We can move it somewhere else and reposition.

@jasmussen jasmussen force-pushed the try/g2 branch from 0417c92 to 503fbbf Jan 10, 2020
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 10, 2020

Alright, did some cleanup and made progress. Improved the focus styles:

placeholder

Unified them with block selection styles:

multi select

Improved positioning of resize handles in a more generic way:

Screenshot 2020-01-10 at 13 05 41

Optical balancing of icon hit areas in toolbar, rewritten to work with any component groups:

Screenshot 2020-01-10 at 13 07 36

Still on the ToDo:

  • button focus styles refactor and consistency + toggle states
  • reusable blocks
  • theme colors, where and when to use (for example it has been a problem to use red/danger-color buttons in midnight themes)
  • color variable cleanup to colors.scss
  • navigation mode styles
  • new dropdown icon for inline styles
  • fix switcher for icons with no switching, like nav items
  • remove todos
  • problem with the toolbar needing to be detached so it can align left and right
  • we need to refactor away the block-edit::before border
  • test plugin toolbar groups and plugins that add to the inline slot

So there's still a ways to go for this branch to be ready to merge.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 13, 2020

@jasmussen in the format toolbar, when you click the buttons, you'll notice that the focus style is not centered properly and that there's jumpiness when you "toggle" formats.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 13, 2020

Yep, that's one of the things I mean to look at now.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 13, 2020

Refactored focus, hover and toggled states for the block toolbar today:

focus

Screenshot 2020-01-13 at 12 46 54

For this particular effort, I'm trying to unify on a single visual style (2px rounded) that is the same for:

  • block focused
  • block selected
  • button focused

Aside from being a single identifiable focus style, it works well with things that are bordered, such as the placeholder and text input fields, in that the 1px to 2px border becomes a shape change so that it does not need to also have contrast change.

However this is a change from the 1px style, and while I vastly prefer this personally, I could use a sanity check.

Similarly — we need a visual treatment for a button that is toggled and focused. Here's what I have so far:

Screenshot 2020-01-13 at 12 47 19

That is, a white inner halo. However in being inset, that makes the icon square. Can we do better? CC: @mtias @pablohoneyhoney

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 13, 2020

I like the code refactoring, I personally prefer a 1px focus style design-wise though.

@jasmussen jasmussen force-pushed the try/g2 branch from 2fb9264 to 1d53509 Jan 23, 2020
@@ -25,6 +25,14 @@ $light-gray-200: #f3f4f5;
$light-gray-100: #f8f9f9;
$white: #fff;

// G2 colors.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 23, 2020

Author Contributor

I have the same remark about these variables. I think we should avoid using "gray", "blue" in these. Even "dark", "light" I'm not sure about them.

I'd prefer if we use semantic variables. "primary-border-color"... That way when we decide to change the color itself, we won't have to change the variable name. It won't go out of sync.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Jan 23, 2020

Contributor

I see that point. @pablohoneyhoney any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.