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

implement support for the width query #14192

Merged
merged 1 commit into from Nov 26, 2016
Merged

implement support for the width query #14192

merged 1 commit into from Nov 26, 2016

Conversation

@gterzian
Copy link
Member

gterzian commented Nov 13, 2016 •
edited by larsbergstrom

implement support for the width media query

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

This change is Reviewable
@highfive
Copy link

highfive commented Nov 13, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/media_queries.rs
  • @emilio: components/style/media_queries.rs
@gterzian
Copy link
Member Author

gterzian commented Nov 13, 2016 •
edited

The test in https://github.com/servo/servo/pull/14192/files#diff-f199ebf0b50ca2e217e93e4f1b0d17bd is now passing, however I wonder if it's really 'that easy'...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 13, 2016

☔ The latest upstream changes (presumably #14174) made this pull request unmergeable. Please resolve the merge conflicts.
@emilio
emilio requested changes Nov 13, 2016
View changes
Copy link
Member

emilio left a comment

Yup, it was sort of "that easy" :P

Still a few nits here and there, but looks good over-all :)
components/style/media_queries.rs Outdated
@@ -146,6 +146,7 @@ impl ToCss for MediaQuery {
let (mm, l) = match e {
Expression::Width(Range::Min(ref l)) => ("min", l),
Expression::Width(Range::Max(ref l)) => ("max", l),
Expression::Width(Range::Eq(ref l)) => ("eq", l),

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 13, 2016

Member

This would serialize the query as eq-width: foo, which isn't what is supposed to happen (it should be serialized just as width: foo.

Please fix it and write a test for it :)
tests/wpt/mozilla/meta/css/matchMedia.html.ini Outdated
@@ -8,8 +8,7 @@
expected: FAIL

[MediaQueryList.matches for "(width: 200px)"]
expected: FAIL
expected: PASS

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 13, 2016

Member

Passing is the default, so you can remove the whole block.
components/style/media_queries.rs Outdated
@@ -42,7 +42,7 @@ impl Default for MediaList {
pub enum Range<T> {
Min(T),
Max(T),
//Eq(T), // FIXME: Implement parsing support for equality then re-enable this.
Eq(T), // FIXME: Implement parsing support for equality then re-enable this.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 13, 2016

Member

Probably the fixme should be removed :)
@gterzian
Copy link
Member Author

gterzian commented Nov 14, 2016

@emilio thanks for your feedback, I will address those points next weekend 😄
@gterzian gterzian force-pushed the gterzian:support_equality_constraints branch from 34798a8 to c3f4389 Nov 26, 2016
@emilio
emilio reviewed Nov 26, 2016
View changes
components/style/media_queries.rs Outdated
@@ -129,8 +129,12 @@ impl ToCss for MediaQuery {
let (mm, l) = match e {
Expression::Width(Range::Min(ref l)) => ("min", l),
Expression::Width(Range::Max(ref l)) => ("max", l),
Expression::Width(Range::Eq(ref l)) => ("eq", l),
};
match mm {

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 26, 2016

Member

I think this could look cleaner as follows:

let (mm, l) = match e {
    Expression::Width(Range::Min(ref l)) => ("min-", l),
    Expression::Width(Range::Max(ref l)) => ("max-", l),
    Expression::Width(Range::Eq(ref l)) => ("", l),
};

try!(write!("{}width: ", mm));

What do you think?

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Nov 26, 2016

Author Member

yep, that's a good one 👍
@emilio
Copy link
Member

emilio commented Nov 26, 2016

Looks good otherwise, could you squash your commits?

Thanks!
@gterzian
Copy link
Member Author

gterzian commented Nov 26, 2016

Will do, should I squash all in one, or separate code from tests? @emilio
@emilio
Copy link
Member

emilio commented Nov 26, 2016

Both work for me :)
@gterzian
implement support for the width media query
Loading status checks…
84e9277 
failing test for correct to_css of "width"

separate test for mq and to_css

implement css serialization for Eq

remove PASS test expectation

remove FIXME

simplify serialization
@gterzian gterzian force-pushed the gterzian:support_equality_constraints branch from c5c254c to 84e9277 Nov 26, 2016
@emilio
Copy link
Member

emilio commented Nov 26, 2016

@bors-servo r+

Thanks! :)
@bors-servo
Copy link
Contributor

bors-servo commented Nov 26, 2016

📌 Commit 84e9277 has been approved by emilio
@emilio
emilio approved these changes Nov 26, 2016
View changes
@bors-servo
Copy link
Contributor

bors-servo commented Nov 26, 2016

⌛ Testing commit 84e9277 with merge bcc76f8...
bors-servo added a commit that referenced this pull request Nov 26, 2016
@bors-servo
Auto merge of #14192 - gterzian:support_equality_constraints, r=emilio
Loading status checks…
bcc76f8 
implement support for the width query

<!-- Please describe your changes on the following line: -->
implement support for the `width` media query

---
<!-- 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
- [x] These changes fix ##13670 (github issue number if applicable).

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

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

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

bors-servo commented Nov 26, 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 84e9277 into servo:master Nov 26, 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
@gterzian gterzian deleted the gterzian:support_equality_constraints branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@gterzian @highfive @bors-servo @emilio
You can’t perform that action at this time.