Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/394: Block toolbar improvements #422

Merged
merged 1 commit into from
Jul 6, 2018
Merged

t/394: Block toolbar improvements #422

merged 1 commit into from
Jul 6, 2018

Conversation

oleq
Copy link
Member

@oleq oleq commented Jul 4, 2018

Suggested merge commit message (convention)

Other: Allowed list item's buttons to have an outer, visible box-shadow. Ensured the balloon panel's arrow does not cover panel's children. Closes ckeditor/ckeditor5#5456.


Additional information

Note: This is a piece of the constellation https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5-ui/394 containing the following PRs:

Note: Please build docs and check out all guides/examples when reviewing.

Note: Screenshots from Letters would also help /cc @dkonopka

…ow. Ensured the balloon panel's arrow does not cover panel's children. Closes #394.
Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pjasiun
Copy link

pjasiun commented Jul 4, 2018

It looks good to me too, expect the pilcrow icon. I still feel it does not fit the rest of the interface, which is flat with no serifs. Since it is the most outstanding element of the editor which uses it (like Letters, it is the only icon users always see) we already spend much time to make it fit well. We may open a ticket about it, to improve it, but this ticket was not about it. I think we should bring such change through the back door.

@dkonopka
Copy link
Contributor

dkonopka commented Jul 4, 2018

Let's see new icons & buttons focus in the Letters, it looks nice 👍

And as I said in the past, this pilcrow icon is really stylish, but IMHO it's not fit well here 😢

blocktoolbar

@dkonopka
Copy link
Contributor

dkonopka commented Jul 4, 2018

Should we change focus color for disabled buttons (I mean decreased opacity/lightness)?

screen shot 2018-07-04 at 15 23 19

vs

screen shot 2018-07-04 at 15 26 30

@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2018

It looks good to me too, expect the blockquote icon.

? Did you mean the pilcrow icon?

@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2018

Regarding the pilcrow icon – its fanciness makes it stand out and I'd say that this is for good. The block toolbar's button is inlined on a page where CKEditor 5 is rendered. A more graphical icon makes it more distinctive. IMO, it's a bit more clear that this is a part of the UI. Something in line with what @scofalik mentioned too – that the button is too "flat". This icon may partially help with that.


We may open a ticket about it, to improve it, but this ticket was not about it.

Previously you wrote:

I want to make this ticket more generric to gather whole feedback about the UI of block toolbar in CKE 5.

The button is part of the block toolbar feature, so it's not completely out of the scope.

@pjasiun
Copy link

pjasiun commented Jul 4, 2018

It looks good to me too, expect the blockquote icon.

? Did you mean the pilcrow icon?

Yes, I mean pilcrow icon. Can we move the discussion about it to the separate ticket, since already 3 people said it does not fit?

@oleq
Copy link
Member Author

oleq commented Jul 4, 2018

Should we change focus color for disabled buttons (I mean decreased opacity/lightness)?

Truth to tell, as I browsed Material UI and Bootstrap docs, I noticed that disabled items don't get focus in the first place. They're excluded from the navigation, which could be a solution for the issue (perhaps a followup?).

For now, we can make the outline less visible, as you suggested 👍

@Reinmar
Copy link
Member

Reinmar commented Jul 4, 2018

Can we move the discussion about it to the separate ticket, since already 3 people said it does not fit?

Sure. But not because 3 people said something.

@oleq
Copy link
Member Author

oleq commented Jul 4, 2018

FYI: De-serifed the pilcrow and adjusted the :focus shadows for disabled buttons in respective repos. Another round of review, please ;-)

Copy link
Contributor

@dkonopka dkonopka left a comment

Choose a reason for hiding this comment

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

👍

@pjasiun
Copy link

pjasiun commented Jul 5, 2018

Looks great, feel free to merge it. 👍

@dkonopka dkonopka merged commit 8a64ee2 into master Jul 6, 2018
@dkonopka dkonopka deleted the t/394 branch July 6, 2018 07:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block toolbar UI improvements
5 participants