Live samples use invalid width

mdn / yari

The platform code behind MDN Web Docs

Mozilla Public License 2.0

1.18k stars 501 forks source link

Live samples use invalid width #7562

Open MrBrain295 opened 1 year ago

MrBrain295 commented 1 year ago

Summary

The width for live samples is a percentage which isn’t a valid value.

URL

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

Reproduction steps

Look at the width attribute on a live sample.

Expected behavior

A valid number or CSS.

Actual behavior

An invalid value.

Device

Desktop

Browser

Firefox

Browser version

Stable

Operating system

Linux

Screenshot

No response

Anything else?

No response

Validations

[X] I have read the Community Participation Guidelines.
[X] I have verified that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] I have checked that this is a concrete bug. For Q&A open a GitHub Discussion.

caugner commented 1 year ago

See: https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeveloper.mozilla.org%2Fen-US%2Fdocs%2FWeb%2FHTML%2FElement%2Fdialog

This is backed by the HTML Specification:

The attributes [width and height], if specified, must have values that are valid non-negative integers.

caugner commented 1 year ago

Actually, it looks like we could just deprecate the width parameter of the EmbedLiveSample macro, because live samples always span the full width according to this CSS that takes precedence:

https://github.com/mdn/yari/blob/7cb1d24d04ef2b38aff0f7b4e4e816ecd2aba7e2/client/src/document/index.scss#L314-L334

For example, this live sample has width="150" height="100%", but it is still 100% wide and the height is ignored anyways, because a height of 100% doesn't really make sense.

caugner commented 1 year ago

@teoli2003 @SphinxKnight @wbamberg Any objections against ignoring the width parameter of the EmbedLiveSample?

https://github.com/mdn/yari/blob/7cb1d24d04ef2b38aff0f7b4e4e816ecd2aba7e2/kumascript/macros/EmbedLiveSample.ejs#L5-L13

To do this, we would need to reassign the parameters:

$0 - id (same)
$1 - height (previously $2)
$2 - screenshot url (previously $3)

If parameters $1 and $2 are both numeric (including 100%), we could use $2 as height and $3 as screenshot url.

This would allow "migrating" the macro calls in a backward-compatible way.

wbamberg commented 1 year ago

I thought the width parameter had been ignored since the start of the Yari era.

I think we could just revise the API and update macro calls, it would be better than tricks to keep backward compatibility.

SphinxKnight commented 1 year ago

See also #5016.

I thought the width parameter had been ignored since the start of the Yari era.

I think we could just revise the API and update macro calls, it would be better than tricks to keep backward compatibility.

+1 on this.