Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse Rect<T> some more #17071

Merged
merged 3 commits into from May 30, 2017

Conversation

Projects
None yet
5 participants
@nox
Copy link
Member

commented May 28, 2017

This change is Reviewable

nox added some commits May 27, 2017

Make Rect<T> a struct tuple
It makes no sense to have named fields in some cases, notably to reuse
Rect<T> in BorderRadius<T>.
Refactor BorderRadius and move it to the border modules
BorderRadius now parses itself reusing Rect<T>.
@highfive

This comment has been minimized.

Copy link

commented May 28, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/specified/basic_shape.rs, components/style/values/generics/rect.rs, components/style/values/computed/mod.rs, components/style/gecko/conversions.rs, components/style/properties/longhand/outline.mako.rs and 13 more
  • @emilio: components/layout/display_list_builder.rs, components/style/values/specified/basic_shape.rs, components/style/values/generics/rect.rs, components/style/values/computed/mod.rs, components/style/gecko/conversions.rs and 15 more

@highfive highfive assigned emilio and unassigned wafflespeanut May 28, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 28, 2017

⌛️ Trying commit 862fc4f with merge c73a80b...

bors-servo added a commit that referenced this pull request May 28, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

@emilio

This comment has been minimized.

Copy link
Member

commented May 29, 2017

What's the rationale for converting Rect to a tuple? I find that somewhat confusing, over all because in layout we represent rects as (Origin, Size), which makes the tuple indices very confusing.

@nox

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

What's the rationale for converting Rect to a tuple?

Not all values that behave that way correspond to the four sides of a rectangle. For example as part of border-radius, the values correspond to corners.

@emilio

This comment has been minimized.

Copy link
Member

commented May 30, 2017

@bors-servo r+

Perhaps those should be newtypes... But not going to block on that. r=me

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

📌 Commit 862fc4f has been approved by emilio

@nox

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

⌛️ Testing commit 862fc4f with merge 433d689...

bors-servo added a commit that referenced this pull request May 30, 2017

Auto merge of #17071 - servo:derive-all-the-things, r=emilio
Reuse Rect<T> some more

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17071)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

@bors-servo bors-servo merged commit 862fc4f into master May 30, 2017

3 of 4 checks passed

dependency-ci Checking Dependencies
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.