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

Refactor grid style types and impls #16970

Merged
merged 1 commit into from May 22, 2017
Merged

Refactor grid style types and impls #16970

merged 1 commit into from May 22, 2017

Conversation

@MaloJaffre
Copy link
Contributor

MaloJaffre commented May 20, 2017 •
edited

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

highfive commented May 20, 2017

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

highfive commented May 20, 2017

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
Copy link

highfive commented May 20, 2017

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

bors-servo commented May 20, 2017

☔ The latest upstream changes (presumably #16960) made this pull request unmergeable. Please resolve the merge conflicts.
@MaloJaffre MaloJaffre force-pushed the MaloJaffre:grid_cleanup branch from da48e3a to 219c62a 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

bors-servo commented May 21, 2017

☔ The latest upstream changes (presumably #16973) made this pull request unmergeable. Please resolve the merge conflicts.
@wafflespeanut
wafflespeanut approved these changes May 21, 2017
View changes
Copy link
Member

wafflespeanut left a comment

Nice work! Sorry for not looking at this sooner. Ping me once you've rebased and done the changes, and we'll land this :)
components/style/values/generics/grid.rs Outdated
}

impl ComputedValueAsSpecified for GridLine {
}

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut May 21, 2017

Member

Nit: newline not necessary
components/style/values/specified/grid.rs Outdated
Ok(())
}
}

impl Parse for GridLine {

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut May 21, 2017

Member

Let's also move this Parse impl to generics, since the specified and computed values have the same type anyway.
components/style/values/specified/grid.rs Outdated
/// An `<auto-fit>` keyword allowed only for `<auto-repeat>`
AutoFit,
}

impl Parse for RepeatCount {

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut May 21, 2017

Member

This impl too.
components/style/values/generics/grid.rs Outdated
AutoFit,
}


This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut May 21, 2017

Member

Nit: Please remove this newline.
components/style/values/generics/grid.rs Outdated
}
}


This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut May 21, 2017

Member

Nit: ditto on newline (please check for similar nits)
@MaloJaffre MaloJaffre force-pushed the MaloJaffre:grid_cleanup branch from 219c62a to 122bfaa May 21, 2017
@MaloJaffre
Refactor grid style types and impls
Loading status checks…
72db176 
Fixes #16949.
@MaloJaffre MaloJaffre force-pushed the MaloJaffre:grid_cleanup branch from 122bfaa to 72db176 May 21, 2017
@MaloJaffre
Copy link
Contributor Author

MaloJaffre commented May 21, 2017 •
edited

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

wafflespeanut commented May 22, 2017

Thanks for doing this!

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 22, 2017

@wafflespeanut: 🔑 Insufficient privileges: Not in reviewers
@wafflespeanut
Copy link
Member

wafflespeanut commented May 22, 2017 •
edited

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

bors-servo commented May 22, 2017

📌 Commit 72db176 has been approved by wafflespeanut
@wafflespeanut
Copy link
Member

wafflespeanut commented May 22, 2017 •
edited

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

wafflespeanut commented May 22, 2017

@bors-servo p=173
@bors-servo
Copy link
Contributor

bors-servo commented May 22, 2017

@wafflespeanut: 🔑 Insufficient privileges: and not in try users
@wafflespeanut
Copy link
Member

wafflespeanut commented May 22, 2017

Great, code hasn't been deployed yet (sigh)
@nox
Copy link
Member

nox commented May 22, 2017

@bors-servo p=2
@bors-servo
Copy link
Contributor

bors-servo commented May 22, 2017

⌛ Testing commit 72db176 with merge 594479f...
bors-servo added a commit that referenced this pull request May 22, 2017
@bors-servo
Auto merge of #16970 - MaloJaffre:grid_cleanup, r=wafflespeanut
Loading status checks…
594479f 
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

bors-servo commented May 22, 2017

☀️ 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
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
@MaloJaffre MaloJaffre deleted the MaloJaffre:grid_cleanup branch May 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wafflespeanut wafflespeanut

Assignees

@wafflespeanut wafflespeanut

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@MaloJaffre @highfive @emilio @bors-servo @wafflespeanut @jdm @nox
You can’t perform that action at this time.