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

adding text-justify property syntax #60

Merged
merged 2 commits into from May 8, 2017

Conversation

Projects
None yet
3 participants
@chrisdavidmills
Copy link
Contributor

commented Apr 13, 2017

No description provided.

@jwhitlock
Copy link
Member

left a comment

I'm not sure what the standards are for reviewing these data PRs. I think it is up to the person opening the PR to suggest a reviewer with the domain knowledge to review it, and to get an estimated review date.

@@ -5833,6 +5833,21 @@
"order": "lengthOrPercentageBeforeKeywords",
"status": "standard"
},
"text-justify": {
"syntax": "auto | distribute | inter-character | inter-word | none",

This comment has been minimized.

Copy link
@jwhitlock

jwhitlock Apr 19, 2017

Member

The draft spec defines as auto | none | inter-word | inter-character. Is distribute a valid value? Why change the order?

This comment has been minimized.

Copy link
@chrisdavidmills

chrisdavidmills May 3, 2017

Author Contributor

distribute is a historical value that is still supposed for compatibility reasons (it acts as an alias of inter-word)

"CSS Text"
],
"initial": "auto",
"appliesto": "allElements",

This comment has been minimized.

Copy link
@jwhitlock

jwhitlock Apr 19, 2017

Member

The draft spec says it applies to inline boxes

This comment has been minimized.

Copy link
@chrisdavidmills

chrisdavidmills May 3, 2017

Author Contributor

I've changed it to "inlineLevelAndTableCellElements", as this is the closest thing I could find that is used elsewhere in the file.

"initial": "auto",
"appliesto": "allElements",
"computed": "asSpecified",
"order": "uniqueOrder",

This comment has been minimized.

Copy link
@jwhitlock

jwhitlock Apr 19, 2017

Member

The draft spec says "Canonical order: n/a". Is this the same?

This comment has been minimized.

Copy link
@chrisdavidmills

chrisdavidmills May 3, 2017

Author Contributor

I put it as uniqueOrder, because the newest spec says "n/a", but I don't know if this is right. I honestly don't know this stuff very well. I'm going to suggest that we review this syntax in next weeks' editors meeting

"appliesto": "allElements",
"computed": "asSpecified",
"order": "uniqueOrder",
"status": "standard"

This comment has been minimized.

Copy link
@jwhitlock

jwhitlock Apr 19, 2017

Member

This is confusing for me. There is a [W3C Last Call Working Draft] from 10 Oct 2013, and a Editor's Draft from 19 April 2017. The Editor's draft is the one mentioned on MDN, and changes a few of the items here. It's unclear if this is trying to follow the 2013 spec, the 2017 draft, or is combining them in some way.

This comment has been minimized.

Copy link
@chrisdavidmills

chrisdavidmills May 3, 2017

Author Contributor

We always follow the newest spec, afaik. This is what I tried to do when filling this in.

@jwhitlock

This comment has been minimized.

Copy link
Member

commented May 5, 2017

I'm happy with this, and I think it is ready to merge. I also don't think I should review any more - I don't have the domain knowledge to review the data, and a linter would do a better job of validating JSON and the schema.

@Elchi3

This comment has been minimized.

Copy link
Member

commented May 8, 2017

Thanks, I'm also not a CSS expert, but this looks good to me overall.

@Elchi3 Elchi3 merged commit f02db99 into mdn:master May 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chrisdavidmills chrisdavidmills deleted the chrisdavidmills:text-justify branch May 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.