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

Jump to bottom

Implement border-image shorthand #14189

Merged
merged 2 commits into from Nov 14, 2016
Merged

Implement border-image shorthand #14189

merged 2 commits into from Nov 14, 2016

Conversation

canova
Copy link
Contributor

@canova canova commented Nov 12, 2016 •
edited by larsbergstrom

Implementation of border-image shorthand.
r? Manishearth

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/shorthand/border.mako.rs, components/style/properties/longhand/border.mako.rs
  • @emilio: components/style/properties/shorthand/border.mako.rs, components/style/properties/longhand/border.mako.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 12, 2016
@canova canova changed the title Implement border-image shorthand [WIP] Implement border-image shorthand Nov 12, 2016
Manishearth
Manishearth reviewed Nov 13, 2016
View reviewed changes
components/style/properties/longhand/border.mako.rs
@@ -196,6 +201,14 @@ ${helpers.single_keyword("-moz-float-edge", "content-box margin-box",
computed::LengthOrNumber::Number(0.0))
}

#[inline]
pub fn get_initial_specified_value() -> SpecifiedValue {
SpecifiedValue(vec![LengthOrNumber::Number(Number(0.0)),
Copy link
Member

Choose a reason for hiding this comment

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

Better to represent this as a single number.

Manishearth
Manishearth reviewed Nov 13, 2016
View reviewed changes
components/style/properties/longhand/border.mako.rs
@@ -449,6 +467,14 @@ ${helpers.single_keyword("-moz-float-edge", "content-box margin-box",
computed_value::SingleComputedValue::Number(1.0))
}

#[inline]
pub fn get_initial_specified_value() -> SpecifiedValue {
SpecifiedValue(vec![SingleSpecifiedValue::Number(Number(1.0)),
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Manishearth
Manishearth reviewed Nov 13, 2016
View reviewed changes
components/style/properties/longhand/border.mako.rs
@@ -301,6 +314,11 @@ ${helpers.single_keyword("-moz-float-edge", "content-box margin-box",
computed_value::T(RepeatKeyword::Stretch, RepeatKeyword::Stretch)
}

#[inline]
pub fn get_initial_specified_value() -> SpecifiedValue {
SpecifiedValue(RepeatKeyword::Stretch, Some(RepeatKeyword::Stretch))
Copy link
Member

Choose a reason for hiding this comment

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

None for the second

Manishearth
Manishearth reviewed Nov 13, 2016
View reviewed changes
components/style/properties/longhand/border.mako.rs
pub fn get_initial_specified_value() -> SpecifiedValue {
SpecifiedValue {
corners: vec![PercentageOrNumber::Percentage(Percentage(1.0)),
PercentageOrNumber::Percentage(Percentage(1.0)),
Copy link
Member

Choose a reason for hiding this comment

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

same

Manishearth
Manishearth reviewed Nov 13, 2016
View reviewed changes
components/style/properties/shorthand/border.mako.rs
slice = Some(value);

// Parse border image width and outset, if applicable.
try!(input.try(|input| {
Copy link
Member

Choose a reason for hiding this comment

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

This should be

                        // Parse border image width and outset, if applicable.
                        let maybe width_outset = input.try(|input| {
                            try!(input.expect_delim('/'));

                            // Parse border image width, if applicable.
                            let w = input.try(|input|
                                border_image_width::parse(context, input)).ok();

                            // Parse border image outset if applicable.
                            let o = input.try(|input| {
                                try!(input.expect_delim('/'));
                                border_image_outset::parse(context, input)
                            }).ok();

                            Ok((w, o))
                        });
                        if let Ok(w, o) = maybe_width_outset {
                            width = w;
                            outset = o;
                        }

In case the try fails you do not want width and outset to be written to.

@canova canova changed the title [WIP] Implement border-image shorthand Implement border-image shorthand Nov 13, 2016
@canova
Copy link
Contributor Author

canova commented Nov 14, 2016

r? @Manishearth Could you review when you're available? :)

@Manishearth
Copy link
Member

@bors-servo r+

looks good!

@bors-servo
Copy link
Contributor

📌 Commit e408b0e has been approved by Manishearth

@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 Nov 14, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit e408b0e with merge e9fa69b...

bors-servo pushed a commit that referenced this pull request Nov 14, 2016
Auto merge of #14189 - canaltinova:border-image-shorthand, r=Manishearth
e9fa69b 
Implement border-image shorthand

<!-- Please describe your changes on the following line: -->
Implementation of border-image shorthand.
r? Manishearth

---
<!-- 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

<!-- Either: -->
- [X] There are tests for these changes

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

☀️ 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 e408b0e into servo:master Nov 14, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 14, 2016
@canova canova deleted the border-image-shorthand branch November 15, 2016 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth Manishearth left review comments

Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@canova @highfive @Manishearth @bors-servo @wafflespeanut