New style getters and cleanups in CSSNode #146

2015-10-21T02:58:52-07:00

lucasr commented

2015-10-21 02:58:52 -07:00

(Migrated from github.com)

This PR adds all missing getters for style properties in CSSNode for consistency and because now we have some use cases for it.

Furthermore, I've renamed the existing getters and some setters to follow a more consistent pattern. This involves an API break so I'm considering this part kinda optional (even though I really think we should do it).

Having "Style" in the method names seems redundant given that we currently have setters that implicitly set style and don't have "style" in their name. For example, we have setPadding() but the getter is called getStylePadding(). We should be more consistent about this.

The following methods are renamed:

getStyleWidth() -> getWidth()
getStyleHeight() -> getHeight()
setStyleWidth() -> setWidth()
setStyleHeight() -> setHeight()
getStyleDirection() -> getDirection()
getStylePadding() -> getPadding()

With these changes, the CSSNode API will be more like "all setters/getters members affect style, unless they "Layout" in their name, which sounds simpler and more consistent.

This PR adds all missing getters for style properties in CSSNode for consistency and because now we have some use cases for it. Furthermore, I've renamed the existing getters and some setters to follow a more consistent pattern. This involves an API break so I'm considering this part kinda optional (even though I really think we should do it). Having "Style" in the method names seems redundant given that we currently have setters that implicitly set style and don't have "style" in their name. For example, we have `setPadding()` but the getter is called `getStylePadding()`. We should be more consistent about this. The following methods are renamed: - getStyleWidth() -> getWidth() - getStyleHeight() -> getHeight() - setStyleWidth() -> setWidth() - setStyleHeight() -> setHeight() - getStyleDirection() -> getDirection() - getStylePadding() -> getPadding() With these changes, the CSSNode API will be more like "all setters/getters members affect style, unless they "Layout" in their name, which sounds simpler and more consistent.

kmagiera commented

2015-10-21 03:07:35 -07:00

(Migrated from github.com)

looks good.

@facebook-github-bot import

looks good. @facebook-github-bot import

facebook-github-bot commented

2015-10-21 03:14:33 -07:00

(Migrated from github.com)

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1476275942680136/int_phab to review.

lucasr commented

2015-10-21 06:34:49 -07:00

(Migrated from github.com)

Changed my mind after fiddling a bit more with this new API. I think we should keep the "Style" in getters that are present in both style and layout for clarity e.g. node bounds and layout direction. It still makes sense to rename getStylePadding() to getPadding() as it's a style-only attribute.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#146