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

Refactor grid style types and impls #16970

Merged
merged 1 commit into from May 22, 2017
Merged

Conversation

MaloJaffre
Copy link
Contributor

@MaloJaffre MaloJaffre commented May 20, 2017

Fixes #16949.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because it's just a refactor

This change is Reviewable

@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/specified/grid.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/values/generics/mod.rs
  • @emilio: components/style/values/specified/grid.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/values/generics/mod.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 20, 2017
@highfive
Copy link

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@emilio
Copy link
Member

emilio commented May 20, 2017

r? @wafflespeanut

@highfive highfive assigned wafflespeanut and unassigned emilio May 20, 2017
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #16960) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label May 20, 2017
@MaloJaffre MaloJaffre changed the title Refactor grid style types and impls [WIP] Refactor grid style types and impls May 20, 2017
@MaloJaffre MaloJaffre changed the title [WIP] Refactor grid style types and impls Refactor grid style types and impls May 20, 2017
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #16973) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label May 21, 2017
Copy link
Contributor

@wafflespeanut wafflespeanut left a comment

Choose a reason for hiding this comment

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

Nice work! Sorry for not looking at this sooner. Ping me once you've rebased and done the changes, and we'll land this :)

}

impl ComputedValueAsSpecified for GridLine {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: newline not necessary

Ok(())
}
}

impl Parse for GridLine {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also move this Parse impl to generics, since the specified and computed values have the same type anyway.

/// An `<auto-fit>` keyword allowed only for `<auto-repeat>`
AutoFit,
}

impl Parse for RepeatCount {
Copy link
Contributor

Choose a reason for hiding this comment

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

This impl too.

AutoFit,
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Please remove this newline.

}
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ditto on newline (please check for similar nits)

@MaloJaffre
Copy link
Contributor Author

MaloJaffre commented May 21, 2017

No worries and thanks for your review @wafflespeanut!
I've rebased and applied your comments.

@wafflespeanut
Copy link
Contributor

Thanks for doing this!

@bors-servo r+

@bors-servo
Copy link
Contributor

@wafflespeanut: 🔑 Insufficient privileges: Not in reviewers

@wafflespeanut
Copy link
Contributor

wafflespeanut commented May 22, 2017

Waat Oh shit. Case sensitive 😐

@jdm
Copy link
Member

jdm commented May 22, 2017

@wafflespeanut Homu is case-sensitive; did you rename your account recently?

@bors-servo: r=wafflespeanut

@bors-servo
Copy link
Contributor

📌 Commit 72db176 has been approved by wafflespeanut

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels May 22, 2017
@wafflespeanut
Copy link
Contributor

wafflespeanut commented May 22, 2017

@jdm Yeah, I just realized that. r? servo/saltfs#673

@wafflespeanut
Copy link
Contributor

@bors-servo p=173

@bors-servo
Copy link
Contributor

@wafflespeanut: 🔑 Insufficient privileges: and not in try users

@wafflespeanut
Copy link
Contributor

Great, code hasn't been deployed yet (sigh)

@nox
Copy link
Contributor

nox commented May 22, 2017

@bors-servo p=2

@bors-servo
Copy link
Contributor

⌛ Testing commit 72db176 with merge 594479f...

bors-servo pushed a commit that referenced this pull request May 22, 2017
Refactor grid style types and impls

Fixes #16949.
- [x] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because it's just a refactor

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

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: wafflespeanut
Pushing 594479f to master...

@bors-servo bors-servo merged commit 72db176 into servo:master May 22, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 22, 2017
@MaloJaffre MaloJaffre deleted the grid_cleanup branch May 22, 2017 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants