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 inset property #319

Merged
merged 3 commits into from
Nov 5, 2018
Merged

Conversation

rachelandrew
Copy link
Collaborator

The inset property is missing from the data, I think this may be the cause of the inset() basic shape being used incorrectly here: https://developer.mozilla.org/en-US/docs/Web/CSS/inset#Formal_syntax

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

I think we need a few changes here. In the spec, the syntax is listed as <‘top’>{1,4}.

Usually the syntax for a shorthand property looks more like this: https://github.com/mdn/data/blob/master/css/properties.json#L2233

In this case it looks like it just takes most of the same values as the individual properties? E.g. https://drafts.csswg.org/css-position-3/#propdef-top?

But I think we probably still need to list the associated longhand properties in the initial and computed fields?

@rachelandrew
Copy link
Collaborator Author

which longhand properties though? The t,r,b,l ones or the logical versions. The spec just points to the physical ones.

@rachelandrew
Copy link
Collaborator Author

rachelandrew commented Nov 1, 2018

Also, it looks like the other ones are using left not top, so should I change those as well?

Edited as "other ones" is super vague. The other inset-* properties. It looks like they are specified as top too (not that it actually makes any difference, but for consistency).

@chrisdavidmills
Copy link
Contributor

Basically, the longhand ones that this property is shorthand for. Hrm, you are right, this is vague ;-)

I think i'll probably accept this if you just update the syntax. We can improve it further when we get better information.

@chrisdavidmills
Copy link
Contributor

LGTM. Thanks Rachel.

@chrisdavidmills chrisdavidmills merged commit 0668551 into mdn:master Nov 5, 2018
@rachelandrew rachelandrew deleted the inset-property branch November 5, 2018 19:41
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

2 participants