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
Stylo: Add grid-gap shorthand #16081
Conversation
r? @Manishearth |
Thanks a ton for helping out with this stuff @methyl! This work is critical to helping us ship Servo in Firefox this year. |
|
||
pub fn parse_value(context: &ParserContext, input: &mut Parser) -> Result<Longhands, ()> { | ||
let row_gap = grid_row_gap::parse(context, input)?; | ||
let column_gap = grid_column_gap::parse(context, input).unwrap_or(row_gap.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use input.try()
; otherwise the cursor won't reset after a failed parse.
impl<'a> ToCss for LonghandsToSerialize<'a> { | ||
fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { | ||
self.grid_row_gap.to_css(dest)?; | ||
dest.write_str(" ")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should skip these two steps if the column gap is the same. check what firefox does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the change, although I've been looking around and can't get any idea what's the usecase of ToCss so I can confirm with Firefox behavior.
@Manishearth I think build fails because gaps are declared for gecko, so it won't get registered as longhand. Not sure what to do in this case |
put |
@bors-servo r+ |
📌 Commit c331376 has been approved by |
@bors-servo r- Needs squash. |
c331376
to
6eba653
Compare
Thanks! @bors-servo r=Manishearth |
📌 Commit 6eba653 has been approved by |
Stylo: Add grid-gap shorthand - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors Should I add any tests? <!-- 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/16081) <!-- Reviewable:end -->
☀️ 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 |
./mach build -d
does not report any errors./mach test-tidy
does not report any errorsShould I add any tests?
This change is