Replace alert() in Web/API live samples #7570
Conversation
Preview URLsFlawsNone! 🎉 External URLsURL: No new external URLs (this comment was updated 2021-09-10 00:05:57.363584) |
wbamberg
changed the title
| <div id="output"></div> | ||
| </pre> | ||
|
|
||
| <pre class="brush: css hidden"> |
There was a problem hiding this comment.
What does the hidden do here?
(My gut feeling would be suppressing it from showing up on the page).
In that case, I'd suggest to add a CSS comment that this styling is meant to provide feedback into what's going on behind the scenes.
There was a problem hiding this comment.
I haven't done this because using hidden (which, as you guessed, hides code blocks in the rendered output) is a very common thing to do in MDN live samples, and as a matter of convention we don't signal it using a comment.
| } | ||
|
|
||
| #output:before { | ||
| content: "Output:"; |
There was a problem hiding this comment.
I can see why you did it that way, but the value feels like „non-decorative” to me. I'd suggest to move that to the .textContent assignment instead (for example, using a template literal and interpolate the message).
There was a problem hiding this comment.
Yeah, makes sense.
|
|
||
| #output:before { | ||
| content: "Output:"; | ||
| padding-right: .5rem; |
There was a problem hiding this comment.
Can we use logical properties here?
It's a matter of taste, but I find it harder to read .5 than 0.5.
There was a problem hiding this comment.
:before is removed, so this isn't relevant any more.
| margin: 1rem 0; | ||
| } | ||
|
|
||
| #output:before { |
There was a problem hiding this comment.
| #output:before { | |
| #output::before { |
Personally, I prefer to use two colons to separate pseudo elements from pseudo classes, but both ways are valid.
There was a problem hiding this comment.
:before is removed, so this isn't relevant any more.
* upstream/main: (87 commits) Update formdata and touchstart macros (mdn#7713) Corrected sentence in Streams API overview (mdn#7712) Fix broken link (mdn#7711) Replace SVGMatrix with DOMMatrix (mdn#7710) Removed example covered by other examples (mdn#7706) Remove last link to NameList (mdn#7708) Delete DOMObject (mdn#7701) Remove <dl> list for return value of DOMException() (mdn#7699) Fix broken links in Glossary (mdn#7692) Fix broken link to NSS/FAQ (mdn#7709) Update DOM intro (DOM is no more different in each browsers) (mdn#7705) Update Touch() contructor syntax (mdn#7702) Replace usage of alert() in File.name(mdn#7703) Replace usage of alert() in KeyboardEvent.metaKey(mdn#7704) Replace usage of alert() in DOMException() (mdn#7698) Remove unnecessary <code> tag (mdn#7697) Fix mixin example in Object.create() article (mdn#7631) Replace usage of alert() in EventListener (mdn#7690) Fix link from Element to Document.querySelectorAll (mdn#7688) Replace usage of alert() in Window.find() (mdn#7686) ...
|
Thanks @Ryuno-Ki ! I think I have addressed your comments. |
|
@Ryuno-Ki, given that this PR will "rot" later tonight with the conversion to Markdown, I think we should land this and eventually address problems in follow-ups. I know you are busy these days, so I will proceed to merge it in a few hours if you don't have time to answer. |
This PR should eventually be a fix for #7566.
I don't think it's worth spending a ton of time on this, but it's worth thinking a little bit about having a really simple but usable pattern for replacing
alert()in live samples.So this PR currently implements one approach to this on a single page, so I can get some feedback :). In this approach I just have an
<hr>after the main part of the example, and under that I have an "Output"<div>that I can write output into, instead of putting it in an alert.