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

style: Minor nits on the alignment properties. #19850

Merged
merged 3 commits into from Jan 24, 2018

Conversation

emilio
Copy link
Member

@emilio emilio commented Jan 24, 2018

I'm going to touch this in a bit, let's do it a bit less painful.


This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/specified/align.rs
  • @canaltinova: components/style/values/specified/align.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 24, 2018
@highfive
Copy link

warning Warning warning

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

I'm going to touch this in a bit, let's do it a bit less painful.

MozReview-Commit-ID: LhBNMkUXlUK
@emilio
Copy link
Member Author

emilio commented Jan 24, 2018

r? @nox or @mbrubeck

Copy link
Contributor

@nox nox left a comment

Choose a reason for hiding this comment

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

r=me with an explanation in the ticket, aren't the generated files you changed generated on the Gecko side?

#[link_name = "\u{1}_ZN10nsCSSProps34kAutoCompletionAlignJustifyContentE"]
pub static mut nsCSSProps_kAutoCompletionAlignJustifyContent:
#[link_name = "\u{1}_ZN10nsCSSProps34kAutoCompletionContentDistributionE"]
pub static mut nsCSSProps_kAutoCompletionContentDistribution:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change intended? Please ELI5.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it is not, but it's harmless anyway.

align-content and justify-content will have different types in a second.

MozReview-Commit-ID: 5JDeR5kXZNP
This matches the spec term and, again, the two properties will have different
grammars soon.

MozReview-Commit-ID: 8f8JXj2NnCi
@emilio
Copy link
Member Author

emilio commented Jan 24, 2018

@bors-servo r=nox

  • I reverted the generated changes.

@bors-servo
Copy link
Contributor

📌 Commit 711ea51 has been approved by nox

@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. labels Jan 24, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit 711ea51 with merge f28c77c...

bors-servo pushed a commit that referenced this pull request Jan 24, 2018
style: Minor nits on the alignment properties.

I'm going to touch this in a bit, let's do it a bit less painful.

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

💔 Test failed - arm32

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 24, 2018
@emilio
Copy link
Member Author

emilio commented Jan 24, 2018

@bors-servo retry

  • ARM build failed to checkout?

@bors-servo
Copy link
Contributor

⌛ Testing commit 711ea51 with merge ab00a3e...

bors-servo pushed a commit that referenced this pull request Jan 24, 2018
style: Minor nits on the alignment properties.

I'm going to touch this in a bit, let's do it a bit less painful.

<!-- 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/19850)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 24, 2018
@bors-servo
Copy link
Contributor

💔 Test failed - arm32

@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 24, 2018
@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jan 24, 2018
@emilio
Copy link
Member Author

emilio commented Jan 24, 2018

@bors-servo retry

command timed out: 1200 seconds without output running ['./mach', 'build', '--rel', '--target=arm-unknown-linux-gnueabihf'], attempting to kill

@bors-servo
Copy link
Contributor

⌛ Testing commit 711ea51 with merge e6470a1...

bors-servo pushed a commit that referenced this pull request Jan 24, 2018
style: Minor nits on the alignment properties.

I'm going to touch this in a bit, let's do it a bit less painful.

<!-- 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/19850)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 24, 2018
@bors-servo
Copy link
Contributor

💔 Test failed - arm32

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 24, 2018
@emilio
Copy link
Member Author

emilio commented Jan 24, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Testing commit 711ea51 with merge 7354a32...

bors-servo pushed a commit that referenced this pull request Jan 24, 2018
style: Minor nits on the alignment properties.

I'm going to touch this in a bit, let's do it a bit less painful.

<!-- 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/19850)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 24, 2018
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox
Pushing 7354a32 to master...

@bors-servo bors-servo merged commit 711ea51 into servo:master Jan 24, 2018
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 24, 2018
bors-servo pushed a commit that referenced this pull request Jan 29, 2018
style: Update css-align to the spec (mostly)

This is on top of #19850.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1430817, and updates us
to the current version of the css-align spec.

The only remaining change is the justify-items: auto FIXME.

<!-- 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/19851)
<!-- Reviewable:end -->
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

5 participants