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

Introduce BoxEdges typedef. #1816

Merged
merged 5 commits into from
Jan 24, 2020
Merged

Introduce BoxEdges typedef. #1816

merged 5 commits into from
Jan 24, 2020

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jan 23, 2020

Suggested merge commit message (convention)

Docs: Introduce BoxEdges type definition. Closes ckeditor/ckeditor5#6132.

ps.: I've also removed the TopRifghtBottomLeft nomenclature from methods and used BoxEdges instead. I think that this is way better then previous.


Additional information

@jodator jodator requested a review from oleq January 23, 2020 15:32
@coveralls
Copy link

coveralls commented Jan 23, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling b488001 on i/6132 into b2a8189 on master.

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

A good idea but the word choice is wrong IMO.

* left: '7px'
* }
*
* @typedef {Object} module:engine/view/stylesmap~BoxEdges
Copy link
Member

Choose a reason for hiding this comment

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

I think it should rather be box sides instead. Check out how w3 uses both terms https://www.w3.org/TR/CSS2/box.html (search for both):

These are for edges:

The perimeter of each of the four areas (content, padding, border, and margin) is called an "edge", so each box has four edges:

  • content edge or inner edge
  • padding edge
  • border edge
    ...

image

These are for sides (and IMO this is what we want to say):

The 'padding' shorthand property sets the padding for all four sides while the other padding properties only set their respective side.

If there is only one component value, it applies to all sides. If there are two values, the top and bottom paddings are set to the first value and the right and left paddings are set to the second.

The 'border-color' property can have from one to four component values, and the values are set on the different sides as for 'border-width'.

Summary

So edges are more like contours around the box (containing or not containing the padding, border, etc.) and the box has always 4 sides (top, left, bottom, right).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I've looked at MDN docs and saw the edges used mostly. But your explanaition & wording is better. I'll fix that :)

jodator and others added 2 commits January 24, 2020 09:27
Edges are more like contours around the box (containing or not containing the padding, border, etc.) and the box has always 4 sides (top, left, bottom, right).
@oleq
Copy link
Member

oleq commented Jan 24, 2020

What do we do about https://ckeditor.com/docs/ckeditor5/latest/api/module_utils_dom_getborderwidths.html#static-function-getBorderWidths? It should use the same typedef.

Is it OK if it used the typedef from the engine? Or maybe the typedef should be defined somewhere else?

@jodator
Copy link
Contributor Author

jodator commented Jan 24, 2020

Is it OK if it used the typedef from the engine? Or maybe the typedef should be defined somewhere else?

IDK to be fair... I'll just write what comes to my mind:

  1. it would be nice to have one typedef for that
  2. OTOH we cannot introduce circular dependency engine -> utils -> engine
  3. I can live with no typedef for utils stuff
  4. I've just check and we already use module:engine/view/observer in keystrokehandler docs

...

I'd go with a follow up to iron this out (as the keystroke handler looks like in wrong place)

@oleq oleq merged commit 48bea53 into master Jan 24, 2020
@oleq oleq deleted the i/6132 branch January 24, 2020 10:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants