-
Notifications
You must be signed in to change notification settings - Fork 22.8k
Fix broken playgrounds (and some improvements) in CSS positioning tutorial #39879
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
base: main
Are you sure you want to change the base?
Conversation
17a1d7f
to
d72f06f
Compare
Preview URLs External URLs (2)URL:
|
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.
Hi there @steffahn, and thank you for the significant amount of time and effort you've put into this. On long complex pages like this one, it is always painful when we end up with code examples bleeding into one another because of the heading nesting not being quite what was expected.
This page was written before we implemented the live-sample___sample-name
code fence syntax. For new pages, I use it all the time to avoid just such problems.
I read through the page in detail, and what you've done works nicely. I've only made one specific comment where I noticed that what was being rendered in one live sample didn't seem quite right.
However, I have got a more general comment to made — I think you have gone a bit far with the deduplication of code — putting most of the code in one giant chunk near the top of the page will make it quite hard for future page editors to understand and maintain. Do you think you can re-duplicate the main chunks back to the sections they are relevant to?
Obviously, I still want you to keep the live-sample___sample-name
syntax, to avoid the code leakage, as that is super useful.
Let me know what you think, and thanks again!
background: yellow; | ||
top: 30px; | ||
left: 30px; | ||
} | ||
``` | ||
|
||
{{ EmbedLiveSample('Setting_position_absolute', '100%', 450) }} | ||
If you now save and refresh, you should see something like so: |
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.
The code shown in the next live sample isn't quite right. The positioned box says "Now I'm absolutely positioned relative to the element, not the element!", but it shouldn't, as we haven't got to that stage of changing the positioning context yet.
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.
Oh that's a good catch!!; I missed that! I only saw the "Now I'm absolutely positioned…" start and thought it applied to both. (On the currently live version it does instead still retain the “By default we span 100” text, which was also wrong, and inconsistent with the linked version at https://mdn.github.io/learning-area/css/css-layout/positioning/3_absolute-positioning.html .
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.
Right, yeah, we should probably update those versions too, when the dust settles.
``` | ||
|
||
If you now save and refresh, you should see something like so: | ||
<!-- prettier-ignore-start --> |
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.
You don't have to do this. It's fine for the concatenated code to have nonsensical formatting, but the displayed blocks should have sane indentation (i.e. not start at non-zero)
Yeah, I'll give it another go to re-simplify a bit. |
@steffahn This is still waiting for your update |
Description
Fix broken code examples on https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/CSS_layout/Positioning. And add some improvements.
Motivation
Look e.g. at the current status around https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/CSS_layout/Positioning#introducing_top_bottom_left_and_right, the linked playground there contains code such as
because more code snippets are aggregated into this than what the author anticipated. And this is then of course broken (and results in the
body { … }
rule to be ignored entirely).Also, there are multiple places where a textual description of a very interesting and very significant for understanding
position
intermediate state without any visualization of what happens.Thus I have added 3 such visualizations / playground states, which look as follows:
A displayed version of the
static
code exampleThe not-yet
z-index
flipped state, for visual contrastThe not-yet
margin-top
-fixed state for fixed positioning, also for visual contrastAdditional details
Additionally there are also some minor tweaks here and there. E.g. a
0px
becoming0
for consistency with the snippet that was already displayed; some margin-based height extension for sticky examples using100vh
so now it works on the page and also on the linked playground. And probably more.I probably overdid it a bit with the de-duplicating of shared parts of code snippets. But I've iterated on this long enough now, so I'll just submit the PR now. One user-facing benefit of this piecework is that now also these shortened snippets have gained links to the corresponding playgrounds, e.g.:

Also, the newlines (or lack therof) for some of the generated playgrounds without added things such as single empty lines at the end, or double empty lines at the beginning of some of the hidden code blocks was a bit tricky to work with/around1.
Footnotes
especially since you guys’s
prettier
tooling tended to somewhat silently simply erase all this work from history as soon as one wants to finish their commit. (And I really do mean erase since the way it's in a pre-commit hook which appears to then also add the changes to the work tree, and the staging area and the current commit means the original version of how things were is relatively thoroughly fully eliminated :-/] ↩