-
Notifications
You must be signed in to change notification settings - Fork 12
t/105: Moved rect utilities from BalloonPanelView #106
Conversation
…tersection for provided element/target/limiter/viewport rects.
…nstead of boundingClientRect–like.
R-. |
…ion of rects which has negative width and height (disjoint rects).
Firefox doesn't have |
Actually, we should have |
/** | ||
* Element that is to be positioned. | ||
* | ||
* @member {HTMLElement} module:utils/dom/position~Options#element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to specify the whole path. Just the #element
should be fine (but please do check).
* | ||
* @property {Number} top Top position offset. | ||
* @property {Number} left Left position offset. | ||
* @property {String} name Name of the position. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that's what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Positions are named. This is useful because as the getOptimalPosition
util selects the best position, some changes to the DOM may be needed, i.e. the position needs a proper class to be displayed (like an arrow in BalloonPanelView
which changes with each position).
So, in fact, the knowledge about position geometry (top
, left
) is not enough. We need to know, which of the positioning functions passed to https://github.com/ckeditor/ckeditor5-ui-default/blob/t/131/src/balloonpanel/balloonpanelview.js#L145-L150 has been chosen.
That's why positioning functions are named, like https://github.com/ckeditor/ckeditor5-ui-default/blob/t/131/src/balloonpanel/balloonpanelview.js#L207-L211.
TBH, I don't like it either but I couldn't find any other way to simplify this API. I mean, to pass a number of functions, get the output data out of one of them (the best one) and to know precisely which function returned this output data. Ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that, I didn't mean that it's wrong. I think we can live with it (TBH, I don't have energy to try to find a better solution because this one isn't that bad :D). I just wanted this to be better documented. A single example in this module would do enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the only place where position names look odd is https://github.com/ckeditor/ckeditor5-ui-default/blob/t/131/src/balloonpanel/balloonpanelview.js#L207. But it's reasonable there too. You can both, access the returned value satisfy the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually 2, you could mention there that these functions returns objects implementing this specific interface, just to make it clear.
* TODO: more docs and example. | ||
* | ||
* @param {module:utils/dom/position~Options} options Positioning options object. | ||
* @returns {Object} An object containing CSS `top`, `left` coordinates ready to use with `position: absolute` and the `name` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So isn't the type Position
?
/** | ||
* Creates an instance of rect. | ||
* | ||
* // Rect of an HTMLElement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something broke here with indentation.
Great job. I really like the new utils! |
Closes ckeditor/ckeditor5#4923.