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

Fix issue 15872: remove live sample inclusions in CSS #16013

Merged
merged 13 commits into from
May 14, 2022

Conversation

wbamberg
Copy link
Collaborator

Part of #15872 : this removes the transclusion parameter from all EmbedLiveSample calls in the CSS docs.

@wbamberg wbamberg requested a review from a team as a code owner May 13, 2022 21:24
@wbamberg wbamberg requested review from estelle and removed request for a team May 13, 2022 21:24
@github-actions github-actions bot added the Content:CSS Cascading Style Sheets docs label May 13, 2022
@wbamberg wbamberg changed the title Fix issue 15872 css Remove live sample transclusion in CSS May 13, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 13, 2022

Preview URLs

Flaws

Note! 10 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/CSS/font-variant-ligatures
Title: font-variant-ligatures
on GitHub
Flaw count: 1

  • macros:
    • /en-US/docs/Glossary/contextual_forms does not exist

External URLs

URL: /en-US/docs/Web/CSS/CSS_Positioning/Understanding_z_index/Stacking_context_example_1
Title: Stacking context example 1
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/font-variant
Title: font-variant
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/flex-wrap
Title: flex-wrap
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/@counter-style/pad
Title: pad
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/@counter-style/negative
Title: negative
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/flex-grow
Title: flex-grow
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/box-orient
Title: box-orient
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/font-variant-alternates
Title: font-variant-alternates
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/:-moz-list-bullet
Title: ::-moz-list-bullet
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/font-language-override
Title: font-language-override
on GitHub

No new external URLs


URL: /en-US/docs/Web/CSS/font-variant-ligatures
Title: font-variant-ligatures
on GitHub

No new external URLs

(this comment was updated 2022-05-14 04:40:56.395815)

@wbamberg wbamberg changed the title Remove live sample transclusion in CSS Fix issue 15872: remove live sample inclusions in CSS May 13, 2022
@OnkarRuikar
Copy link
Contributor

In stacking_context_example_1/index.md
overflow

Can we increase the height of div4 to 80px. Like div1 and div3 have 80px.

@OnkarRuikar
Copy link
Contributor

OnkarRuikar commented May 14, 2022

Just a question. You have used different flavors of height arguments in the macros:

{{ EmbedLiveSample('Setting horizontal box orientation', '', 100) }}
{{ EmbedLiveSample('Example', '', '300px') }}
{{ EmbedLiveSample('Setting font ligatures and contextual forms', '', '700') }}

They all work, but what is the preferred way to provide height?

As per https://developer.mozilla.org/en-US/docs/MDN/Structures/Live_samples#live_sample_macros

height
The height of the <iframe> to create, specified in px. This is optional; a reasonable default height will be used if you omit this. Note that if you want to use a specific height, you must also specify the width parameter. If you use only one of them, the default frame size is used.

Shouldn't we be providing widths here? Is the width requirement for mobile screens? In which case width is must?
If this doesn't work like this anymore then should we update the Live_samples document?

@wbamberg
Copy link
Collaborator Author

wbamberg commented May 14, 2022

Just a question. You have used different flavors of height arguments in the macros:

{{ EmbedLiveSample('Setting horizontal box orientation', '', 100) }}
{{ EmbedLiveSample('Example', '', '300px') }}
{{ EmbedLiveSample('Setting font ligatures and contextual forms', '', '700') }}

They all work, but what is the preferred way to provide height?

I don't know. Usually I prefer to omit "px" but not sure it matters.

As per https://developer.mozilla.org/en-US/docs/MDN/Structures/Live_samples#live_sample_macros

height
The height of the <iframe> to create, specified in px. This is optional; a reasonable default height will be used if you omit this. Note that if you want to use a specific height, you must also specify the width parameter. If you use only one of them, the default frame size is used.

Shouldn't we be providing widths here? Is the width requirement for mobile screens? In which case width is must? If this doesn't work like this anymore then should we update the Live_samples document?

width used to have an effect in the old platform but AFAIK it is ignored in Yari.

Yes, we should probably update the meta-docs :). See also mdn/yari#5016.

@wbamberg
Copy link
Collaborator Author

According to mdn/yari#5016 (comment) it looks like we ought to omit "px", although I think it works either way.

Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

So, these things were transclusions of themselves (I don't want to see yari's code to handle this parameter)

@teoli2003 teoli2003 merged commit f961a83 into mdn:main May 14, 2022
@OnkarRuikar
Copy link
Contributor

According to mdn/yari#5016 (comment) it looks like we ought to omit "px", although I think it works either way.

Good find! Looks like we need to update the doc https://developer.mozilla.org/en-US/docs/MDN/Structures/Live_samples#live_sample_macros to remove px. It'll prevent future commits form using `px'.

@teoli2003
Copy link
Contributor

Good find! Looks like we need to update the doc https://developer.mozilla.org/en-US/docs/MDN/Structures/Live_samples#live_sample_macros to remove px. It'll prevent future commits form using `px'.

Yes, please.

hamishwillee pushed a commit to hamishwillee/content that referenced this pull request May 23, 2022
* Fix :-moz-list-bullet

* Fix @counter-style/pad

* Fix @counter-style/negative

* Fix font-variant-ligatures

* Fix flex-wrap

* Fix font-variant

* Fix z-index

* Fix font-variant-alternates

* Fix box-orient

* Fix flex-grow

* Fix font-language-override

* Make div4 a little taller

* Omit px for iframe height
@wbamberg wbamberg deleted the fix-issue-15872-css branch April 5, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:CSS Cascading Style Sheets docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants