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

Extract text emphasis style #20252

Merged
merged 5 commits into from Mar 10, 2018

Conversation

NeverHappened
Copy link
Contributor

@NeverHappened NeverHappened commented Mar 8, 2018

Extracted the text-emphasis-style property out of the inherited_text.mako.rs.
This is a part of #19015


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • ./mach cargo-geckolib check does not report any errors
  • These changes fix Move text-emphasis-style outside of mako #19940 (github issue number if applicable).
  • These changes do not require tests because it's a refactoring

This change is Reviewable

@highfive
Copy link

highfive commented Mar 8, 2018

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

@highfive
Copy link

highfive commented Mar 8, 2018

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/gecko.mako.rs, components/style/properties/shorthand/inherited_text.mako.rs, components/style/values/specified/mod.rs, components/style/values/computed/text.rs and 2 more
  • @canaltinova: components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/gecko.mako.rs, components/style/properties/shorthand/inherited_text.mako.rs, components/style/values/specified/mod.rs, components/style/values/computed/text.rs and 2 more
  • @emilio: components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/gecko.mako.rs, components/style/properties/shorthand/inherited_text.mako.rs, components/style/values/specified/mod.rs, components/style/values/computed/text.rs and 2 more

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

highfive commented Mar 8, 2018

warning Warning warning

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

@NeverHappened NeverHappened changed the title Extract text emphasis style WIP Extract text emphasis style Mar 8, 2018
@NeverHappened NeverHappened changed the title WIP Extract text emphasis style Extract text emphasis style Mar 8, 2018
Keyword(KeywordValue),
/// `none`
None,
/// String (will be used only first character) for the text-emphasis-style property
Copy link
Contributor

Choose a reason for hiding this comment

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

"character" is a bit ambiguous here, because "character" in CSS may have different meaning than that in Rust. It should be "grapheme cluster" instead for better exactness. One may think we can use char instead if there is only one "character" being used, which is not true, because a grapheme cluster can well include multiple chars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation

@upsuper
Copy link
Contributor

upsuper commented Mar 9, 2018

Reviewed 7 of 7 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


components/style/properties/shorthand/inherited_text.mako.rs, line 36 at r1 (raw file):

            Ok(expanded! {
                text_emphasis_color: unwrap_or_initial!(text_emphasis_color, color),
                text_emphasis_style: unwrap_or_initial!(text_emphasis_style, style), // TODO: what?

What's the TODO for?


components/style/values/computed/mod.rs, line 78 at r1 (raw file):

pub use self::table::XSpan;
pub use self::text::{InitialLetter, LetterSpacing, LineHeight, MozTabSize};
pub use self::text::{TextAlign, TextOverflow, WordSpacing, TextEmphasisStyle};

TextEmphasisStyle should be put between TextAlign and TextOverflow.


components/style/values/computed/text.rs, line 18 at r1 (raw file):

use values::generics::text::MozTabSize as GenericMozTabSize;
use values::generics::text::Spacing;
use values::specified::text::{TextOverflowSide, TextDecorationLine, FillMode, ShapeKeyword};

Could you order it alphabetically?


components/style/values/computed/text.rs, line 163 at r1 (raw file):

#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
pub struct KeywordValue {

The comment for specified value applies here as well.


components/style/values/specified/text.rs, line 530 at r1 (raw file):

#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
pub enum KeywordValue {

This should be TextEmphasisKeywordValue or something like that. I don't think it is correct to use KeywordValue as name for a type just for text-emphasis in module text.

Alternatively, you can probably create a nested module to wrap all these types in.


components/style/values/specified/text.rs, line 555 at r1 (raw file):

#[derive(Clone, Copy, Debug, MallocSizeOf, Parse, PartialEq, ToCss)]
pub enum FillMode {

Same issue here.


components/style/values/specified/text.rs, line 561 at r1 (raw file):

#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, Parse, PartialEq, ToCss)]
pub enum ShapeKeyword {

And here.


Comments from Reviewable

@upsuper
Copy link
Contributor

upsuper commented Mar 9, 2018

Thanks for your contribution! Please address the review comments.

@jdm jdm assigned upsuper and unassigned metajack Mar 9, 2018
@NeverHappened
Copy link
Contributor Author

Fixed the issues with naming and imports order

@upsuper
Copy link
Contributor

upsuper commented Mar 10, 2018

@bors-servo r+

Thanks!

@bors-servo
Copy link
Contributor

📌 Commit a1dd888 has been approved by upsuper

@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 Mar 10, 2018
@bors-servo
Copy link
Contributor

⌛ Testing commit a1dd888 with merge aa8fb3a...

bors-servo pushed a commit that referenced this pull request Mar 10, 2018
…psuper

Extract text emphasis style

<!-- Please describe your changes on the following line: -->
Extracted the text-emphasis-style property out of the inherited_text.mako.rs.
This is a part of #19015

---
<!-- 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
- [X] `./mach cargo-geckolib check` does not report any errors
- [X] These changes fix #19940 (github issue number if applicable).

<!-- Either: -->
- [X] These changes do not require tests because it's a refactoring

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/20252)
<!-- 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-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: upsuper
Pushing aa8fb3a to master...

@bors-servo bors-servo merged commit a1dd888 into servo:master Mar 10, 2018
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 10, 2018
@NeverHappened NeverHappened deleted the extract_text_emphasis_style branch March 10, 2018 09:17
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.

Move text-emphasis-style outside of mako
5 participants