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.

Sign up for GitHub

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

contain-intrinsic- size, width, height #607

Merged
merged 9 commits into from
Oct 12, 2022
Merged

contain-intrinsic- size, width, height #607

merged 9 commits into from
Oct 12, 2022

Conversation

hamishwillee
Copy link
Contributor

@hamishwillee hamishwillee commented Sep 26, 2022 •
edited
Loading

This adds the data for contain-intrinsic-size, contain-intrinsic-width (will extend it to support contain-intrinsic-height once width is "approved") and then add contain-intrinsic-block-size and contain-intrinsic-inline-size

Note, I have never done one of these before so sorry if things very wrong.

Note this depends on #606, which is cause of the lint error

This is part of mdn/content#20876

hamishwillee
hamishwillee commented Sep 26, 2022
View reviewed changes
css/properties.json Outdated Show resolved Hide resolved
hamishwillee
hamishwillee commented Sep 26, 2022
View reviewed changes
css/properties.json Outdated Show resolved Hide resolved
hamishwillee
hamishwillee commented Sep 26, 2022
View reviewed changes
css/properties.json Outdated Show resolved Hide resolved
@hamishwillee hamishwillee mentioned this pull request Sep 26, 2022
Closed
7 tasks
@hamishwillee
Copy link
Contributor Author

NOTE, at least some of this stuff now appears to be coming from somewhere - is appearing in my local builds.

wbamberg
wbamberg requested changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

I hope this is somewhat helpful! tbh I don't understand a lot of this and there seems to be a lot of cargo-culting going on.

I would love to start generating this from webref so we can stop maintaining this.

css/properties.json Show resolved Hide resolved
css/properties.json Show resolved Hide resolved
css/properties.json Show resolved Hide resolved
css/properties.json Show resolved Hide resolved
css/properties.json Outdated Show resolved Hide resolved
css/properties.json Show resolved Hide resolved
css/properties.json Outdated Show resolved Hide resolved
css/properties.json Show resolved Hide resolved
@hamishwillee
Copy link
Contributor Author

Thanks @wbamberg . I've made the suggested changes and added in the other properties, so this should be good to go. Note though that #607 needs to merge first because of the new enum values.

@hamishwillee
Copy link
Contributor Author

Arrggg. Media is actually required @wbamberg . I'll try add in when I'm back on Friday. Or can we remove it from the schema?

@hamishwillee
Copy link
Contributor Author

@wbamberg I added back the media since removing it causes failures. This can't merge yet though until #608 is merged.

Can you merge both please?

@hamishwillee
Copy link
Contributor Author

@teoli2003 Is this something you might be able to review?

@hamishwillee
Copy link
Contributor Author

hamishwillee commented Oct 11, 2022 •
edited
Loading

@teoli2003 Thanks. Can you merge it too please? I have no rights here.

wbamberg
wbamberg approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Sorry to be slow Hamish.

@wbamberg
Copy link
Contributor

wbamberg commented Oct 12, 2022 •
edited
Loading

Actually @hamishwillee we're still getting lint errors about media.

@wbamberg
Copy link
Contributor

I just filed #611 which ought to make media optional.

@teoli2003 teoli2003 merged commit 3b1c5b5 into mdn:main Oct 12, 2022
@hamishwillee hamishwillee deleted the contain-intrinsic branch October 13, 2022 22:30
@hamishwillee
Copy link
Contributor Author

Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wbamberg wbamberg wbamberg approved these changes

@teoli2003 teoli2003 teoli2003 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hamishwillee @wbamberg @teoli2003