t/131: Contextual toolbar sample #135
Conversation
| // | Balloon | | ||
| // +-----------------+ | ||
| forwardSelection: ( targetRect, balloonRect ) => ( { | ||
| top: targetRect.bottom + 15, |
There was a problem hiding this comment.
https://github.com/ckeditor/ckeditor5-ui-default/blob/t/131/src/balloonpanel/balloonpanelview.js#L19 I forgot to make these public. Static class properties?
| * @default 30 | ||
| * @member {Number} module:ui/balloonpanel/balloonpanelview~BalloonPanelView.arrowHorizontalOffset | ||
| */ | ||
| BalloonPanelView.arrowHorizontalOffset = arrowHorizontalOffset; |
There was a problem hiding this comment.
Why are you exposing these values if you're not reading it later on but use the consts? This means that if someone changes the theme and try to change these values it's not going to work.
| forwardSelection: ( targetRect, balloonRect ) => ( { | ||
| top: targetRect.bottom + arrowVOffset, | ||
| left: targetRect.right - balloonRect.width / 2, | ||
| name: 's' |
There was a problem hiding this comment.
I'm thinking now... how does this value affect anything? It certainly isn't used anywhere in this code. So perhaps it doesn't need to be defined at all? Am I right that only a feature which wants to understand which position the getOptimalPosition function had returned needs to define it?
There was a problem hiding this comment.
OK, I see that these values are also used in themes. So you should also update BallonPanelView#position values. Or even better – extract it to some typedef and document that it's related to the theme's classes.
There was a problem hiding this comment.
I'm not quite sure what you expect me to change. Positions are documented https://github.com/ckeditor/ckeditor5-utils/blob/master/src/dom/position.js#L263-L272. A position must have a name for any code using the getOptimalPosition utility to know, which of the positions have been chosen, not only what coordinates represent it. We've discussed it already.
There was a problem hiding this comment.
What I'm expecting is that the set of supported names is defined somewhere. Initially I thought that this set is totally open, but it's not – the names must much the positions supported by the theme. So we should indicate (to help the developer understanding what it all is) that this is a set of 6 values and that you can add more if you style them.
|
The same problem can be reproduced in Safari. |
…are not overwritable.
…observer is required.
|
As for the Safari, the editor won't start in this browser and it has nothing to do with the scope of the PR. The question is: where's an issue for that? |
Update ckeditor5-dev-tests. It has been fixed. |
No improvement.
|
|
That's exactly the issue I fixed recently and it WFM:
Make sure to update ckeditor5 before and then reinstall ckeditor5-dev-tests. |
|
That was the issue: https://github.com/ckeditor/ckeditor5-dev-tests/issues/40. And the code landed in 3.1.1. |
|
...and hasn't been updated on master |
^3.1.0 satisfies 3.1.1. So everything was up to date. |
|
So no idea then. Updated all packages, |
I know – that's why |
|
OK. Now I got it. It works in Safari. But the whole npm thing is weird, really. The problem will return sooner or later. |
It returns every day. It's how npm works and either we learn how to live with it or quit or jobs ;P We can also consider Yarn, but it didn't work in our case previously. |
|
Toolbar should disappear when selection changes:
|
This is but a sample, an entry point to actual implementation. There are still many behaviors that will need to be implemented, and this is just one of them. The sample should show the idea as simple as possible; it's up to actual integration to make it perfect. So I don't think this belongs to the scope of this PR. |
|
Hm... OK. Let's polish the behaviour in actual usage places. |
It's a demo of #131.
Depends on #98 and ckeditor/ckeditor5-theme-lark#55.