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

Introduced FakePanels for ContextualBalloon. #498

Merged
merged 15 commits into from
May 22, 2019
Merged

Introduced FakePanels for ContextualBalloon. #498

merged 15 commits into from
May 22, 2019

Conversation

oskarwrobel
Copy link
Contributor

@oskarwrobel oskarwrobel commented May 15, 2019

Suggested merge commit message (convention)

Other: Improved UX of ContextualBalloon with multiple stacks. Closes ckeditor/ckeditor5#5509.


Additional information

Required: ckeditor5-theme-lark#cf/2808 ckeditor/ckeditor5-theme-lark#229

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@oskarwrobel
Copy link
Contributor Author

@dkonopka JS part is done. You can make it beautiful now :)

@oskarwrobel oskarwrobel requested a review from pjasiun May 15, 2019 09:20
@dkonopka
Copy link
Contributor

I've added some default styles - polishing in progress.

One thing to fix - we should display one more less fake panels.

Second thought: shouldn't it be named as fakePanel ?

We need to remove here one fake panel

Screenshot 2019-05-16 at 09 56 44

Screenshot 2019-05-16 at 09 55 49

@pjasiun
Copy link

pjasiun commented May 16, 2019

It might be because of the shadow, but the first the space between the real panel and the first fake panel looks smaller than spaces between other fake panels:

Screenshot 2019-05-16 at 11 30 34

Maybe we should add shadow to all panels or move it to the last one?

Also, I believe we need some animation to make the impression that panels switches. Maybe they should move behind the main panel and back to their positions when switching? Or... something. Anything ;)

fake-panels-needs-animation

Also, maybe we should try to add some transparency to these fake panels?

@pjasiun
Copy link

pjasiun commented May 16, 2019

Also, I think you should create a dedicated ticket in https://github.com/ckeditor/ckeditor5-ui/ repository for this issue.

@dkonopka
Copy link
Contributor

Opacity for panels

I was trying to make it bigger/smaller, but it just looks bad:
Screenshot 2019-05-16 at 16 08 46

Weird offset of the first panel

It was caused because additional annotation box-shadow, I've removed it and it looks a bit better.

The second small reason of the wrong position of fake panels is that precision of top/left property (I mean values after comma 931.378px, there was some issue about rounding it to 931px - because Chrome and Firefox are rendering it a bit differently).

Screenshot 2019-05-16 at 16 01 28

Anyway, it looks good right now:

fake-panels

@oskarwrobel oskarwrobel marked this pull request as ready for review May 21, 2019 10:20
@coveralls
Copy link

coveralls commented May 21, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 88da8a7 on cf/2808 into 600b415 on master.

@pjasiun
Copy link

pjasiun commented May 21, 2019

Maybe we could even limit it to 3 panels (1 real panel and 2 fake panels). Now, when 4 panels are displayed (1 real and 3 fake) it looks like too many.

@Reinmar
Copy link
Member

Reinmar commented May 21, 2019

I'm a little late to the party, but is it actually necessary that we display those other balloons at all? After all, there's a switcher at the top so it's hard to miss that. Plus, it's not actually super correct how we display them (e.g. that all of them change their height when you go to the next one and that their placement is a bit virtual too).

Don't get me wrong – it's a really nice touch and could be made really useful (e.g. clicking/hovering the ones below could somehow show a preview of their content). I'm rather worried whether this isn't an overkill for now.

@oskarwrobel
Copy link
Contributor Author

TBH I'm not a fan of these panels in the current shape too. Besides, it doesn't look good for small balloons like a toolbar.

@pjasiun
Copy link

pjasiun commented May 22, 2019

We agreed that we will:

  • - limit the number of fake panels to 2 (1 real panel + 2 fake panel),
  • - make the space between panel smaller, more subtele,
  • - round the position of the main panel to full pixels to fix the issue with misaligned panels,
  • - do not remove "x of x" label for small balloon (it will make them bigger and maybe panels will look there better too),
  • - add an animation of appearing these panels.

@pjasiun
Copy link

pjasiun commented May 22, 2019

⏫ cc @dkonopka

@pjasiun pjasiun merged commit abd05b6 into master May 22, 2019
@pjasiun pjasiun deleted the cf/2808 branch May 22, 2019 14:34
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.

Improve UX of ContextualBalloon with multiple stack
5 participants