-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
docs(fix): Improve <source>
element page for consistency and clarity
#29946
Conversation
Preview URLs (comment last updated: 2023-11-07 15:25:17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dipikabh great work, nice one Dipika. This is much better. I've left you a few content suggestions.
2. A width descriptor, which consists of a string containing a positive integer directly followed by `"w"`, such as `300w`. The default value, if missing, is the infinity. | ||
3. A pixel density descriptor, that is a positive floating number directly followed by `"x"`. The default value, if missing, is `1x`. | ||
- A URL for an image. | ||
- An optional width descriptor—a positive integer directly followed by `"w"`, such as `300w`. If not specified, the value used by default is infinity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- An optional width descriptor—a positive integer directly followed by `"w"`, such as `300w`. If not specified, the value used by default is infinity. | |
- An optional width descriptor—a positive integer directly followed by `"w"`, such as `300w`. If not specified, the value used by default is `infinity`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion led me to check the spec (should have checked earlier). I did not find a reference either in the source element page or in the linked srcset attribute page.
This section in the spec from "srcset attribute" does not mention any default values for the width and pixel density descriptors. I am not sure if we should mention them in our docs. What do you think?
Zero or one of the following:
A width descriptor, consisting of: ASCII whitespace, a valid non-negative integer giving a number greater than zero representing the width descriptor value, and a U+0077 LATIN SMALL LETTER W character.
A pixel density descriptor, consisting of: ASCII whitespace, a valid floating-point number giving a number greater than zero representing the pixel density descriptor value, and a U+0078 LATIN SMALL LETTER X character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, this is a good point. The default value descriptions aren't that helpful and probably confuse more than they inform. About the closest there is to a default value description in the spec is on the srcset
page:
For the purpose of this requirement, an image candidate string with no descriptors is equivalent to an image candidate string with a 1x descriptor.
A bigger issue, in my mind, is that the text here doesn't really describe the purpose of srcset
/sizes
, or how you would use them.
I would suggest:
- Removing the default value descriptions, or perhaps just say something afterwards to the effect that, if a
...w
or...x
is not included, it is equivalent to specifying1x
. I think that is what the spec is saying? - Include some more description and usage examples on the page, or link to some somewhere else. I did attempt to provide this in my learning area Responsive Images article, but I don't know how bulletproof that actually is. It seems to make sense to me, but we've had quite a few tickets filed in the past querying whether the code is correct. It was challenging to figure this stuff out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time and for offering suggestions!
- Agree, removed the default value statement from each descriptor and added a common one in the subsequent para (c96683a).
- Should we tackle this outside this PR? :)
Thanks a lot @chrisdavidmills for taking the time to review this PR. Great feedback, I've accepted your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dipikabh Thanks, this is looking nearly perfect now. There is just the question of what to do about the srcset
description. I have provided some suggestions.
@chrisdavidmills, I've accepted your suggestion for the descriptor default values. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing @dipikabh, that sounds like a very sensible way to tackle this. Approving.
Thanks, @chrisdavidmills! |
Description
This PR aims to improve the
<source>
element documentation by making the following updates:srcset
with a bulleted list.Motivation
To improve the readability and consistency
Additional details
Doc issue tracker: #29788