Skip to content

/ servo

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

style: Add a "start_end()" method to LogicalMargin #14136

Merged
merged 1 commit into from Nov 10, 2016
Merged

style: Add a "start_end()" method to LogicalMargin #14136

merged 1 commit into from Nov 10, 2016

Conversation

@stshine
Copy link
Contributor

stshine commented Nov 8, 2016 •
edited

Add a LogicalMargin::start_end() method that receives a `Direction' parameter. This is useful for some layout that is symmetric in inline and block directions, like flexbox.

Part of #14123.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because refactoring

r? @SimonSapin

This change is Reviewable
@highfive
Copy link

highfive commented Nov 8, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/logical_geometry.rs
  • @emilio: components/layout/fragment.rs, components/style/logical_geometry.rs, components/layout/table_wrapper.rs, components/layout/block.rs, components/layout/table.rs, components/layout/table_row.rs, components/layout/flex.rs, components/layout/model.rs, components/layout/table_cell.rs, components/layout/multicol.rs
@highfive
Copy link

highfive commented Nov 8, 2016

warning Warning warning

  • These commits modify style and layout code, but no tests are modified. Please consider adding a test!
@stshine
Copy link
Contributor Author

stshine commented Nov 8, 2016

Sadly that I failed to find a better name after months :)
@SimonSapin
Copy link
Member

SimonSapin commented Nov 8, 2016

I’m ok with adding a new method that takes a direction parameter, for future code where this direction will be a variable. I think start_end is a better name for this method. (This name by itself is ambiguous, but taking a direction argument clarifies it IMO.)

But I think this method should not replace the existing inline_start_end and block_start_end methods. When the direction is fixed for a particular call, this new method is more verbose and requires a extra use import that was not necessary before. In other words… please revert most of this PR. Sorry to say this after you’ve done all this work.
@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2016

☔ The latest upstream changes (presumably #14130) made this pull request unmergeable. Please resolve the merge conflicts.
@stshine
Copy link
Contributor Author

stshine commented Nov 9, 2016

I have no problem on the rework, while my main concern is the accidental complexity we may add. A new comer may be confused by the three methods with nearly identical behavior, and IMHO we should try to maintain a minimal interface, and the extra use is not a big issue compare to this.

Looking forward to your thoughts on this.
@SimonSapin
Copy link
Member

SimonSapin commented Nov 9, 2016

It’s true that I haven’t been a newcomer to Servo for some time, but I don’t see how ".block_start_end() short for .start_end(Direction::Block)" (and similarly for inline) is confusing.

we should try to maintain a minimal interface, and the extra use is not a big issue compare to this.

I have the exact opposite opinion. These methods are used many times, so making usage more convenient is worth maintaining a couple of one-liner convenience methods.
@stshine
Copy link
Contributor Author

stshine commented Nov 10, 2016

Alright, I agree your perspective has its rationality. I will rebase and follow your comments later.
@stshine stshine force-pushed the stshine:orthogonal-symmetry branch from 5b9167e to 425b9dc Nov 10, 2016
@stshine
style: Add a "start_end()" method to LogicalMargin
Loading status checks…
eb22d33 
Add a `LogicalMargin::start_end()` method that receives a `Direction'
parameter. This is useful for some layout that is symmetric in inline
and block directions, like flexbox.
@stshine stshine force-pushed the stshine:orthogonal-symmetry branch from 425b9dc to eb22d33 Nov 10, 2016
@stshine stshine changed the title style: Unify "*start_end()" methods of LogicalMargin into one style: Add a "start_end()" method to LogicalMargin Nov 10, 2016
@stshine
Copy link
Contributor Author

stshine commented Nov 10, 2016

Comments addressed. @SimonSapin Thanks for review!
@SimonSapin
Copy link
Member

SimonSapin commented Nov 10, 2016

@bors-servo r+

Thanks!

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2016

📌 Commit eb22d33 has been approved by SimonSapin
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2016

⌛ Testing commit eb22d33 with merge 0ef557b...
bors-servo added a commit that referenced this pull request Nov 10, 2016
@bors-servo
Auto merge of #14136 - stshine:orthogonal-symmetry, r=SimonSapin
Loading status checks…
0ef557b 
style: Add a "start_end()" method to LogicalMargin

<!-- Please describe your changes on the following line: -->

Add a `LogicalMargin::start_end()` method that receives a `Direction' parameter. This is useful for some layout that is symmetric in inline and block directions, like flexbox.

Part of #14123.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

r? @SimonSapin

<!-- 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/14136)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2016

💔 Test failed - linux-rel-wpt
@emilio
Copy link
Member

emilio commented Nov 10, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2016

⌛ Testing commit eb22d33 with merge d16f312...
bors-servo added a commit that referenced this pull request Nov 10, 2016
@bors-servo
Auto merge of #14136 - stshine:orthogonal-symmetry, r=SimonSapin
Loading status checks…
d16f312 
style: Add a "start_end()" method to LogicalMargin

<!-- Please describe your changes on the following line: -->

Add a `LogicalMargin::start_end()` method that receives a `Direction' parameter. This is useful for some layout that is symmetric in inline and block directions, like flexbox.

Part of #14123.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

r? @SimonSapin

<!-- 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/14136)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 10, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit eb22d33 into servo:master Nov 10, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@stshine stshine deleted the stshine:orthogonal-symmetry branch Nov 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@stshine @highfive @SimonSapin @bors-servo @emilio
You can’t perform that action at this time.