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.

Sign up for GitHub

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(css): update 'display' interactive demo #2710

Merged
merged 4 commits into from
Jan 16, 2024
Merged

fix(css): update 'display' interactive demo #2710

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab4ab8d
fix(css): update 'display' interactive demo
OnkarRuikar Jan 10, 2024
7061cb5
Apply suggestions from code review
OnkarRuikar Jan 12, 2024
b02b5d0
Update live-examples/css-examples/basic-box-model/display.css
OnkarRuikar Jan 12, 2024
bef7553
add short description in the output
OnkarRuikar Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion live-examples/css-examples/basic-box-model/display.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@
height: 100%;
}

code {
background: #8888;
}

#example-element {
border: 3px solid #00f;
border: 3px solid orange;
OnkarRuikar marked this conversation as resolved.
Show resolved Hide resolved
}

.child {
Expand Down
8 changes: 5 additions & 3 deletions live-examples/css-examples/basic-box-model/display.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,13 @@
<div id="output" class="output large hidden">
<section id="default-example" class="default-example">
<div class="example-container">
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we need to be more explicit.

@OnkarRuikar, should we add this then:

Suggested change
<div id="output" class="output large hidden">
<section id="default-example" class="default-example">
<div class="example-container">
<div id="output" class="output large hidden">
<p>The <code>display</code> property is set on the <code>div</code> element enclosed by the orange dashed border.
You can apply different <code>display</code> values on this element to observe their effects on its layout
in relation to adjacent elements.
Note that this element contains three child <code>div</code> elements.
<section id="default-example" class="default-example">
<div class="example-container">

This is how it would look:

display-inetractive

Copy link
Contributor

@dipikabh dipikabh Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@OnkarRuikar I didn't realize you opened another PR to add the statement :)

Copy link
Contributor

@dipikabh dipikabh Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, that PR is in mdn/content.

I have not noticed other instances where we discuss or refer to the "Try it" demo examples in content.

It might be best to keep the interactive demo examples self-contained. Let me know what you think - we can close mdn/content#31672 and instead make the explanatory updates (as suggested above) to the code within this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dipikabh that's too much text in the output. On narrow screens, readers will have to scroll to see the actual target div.

I have not noticed other instances where we discuss or refer to the "Try it" demo examples in content.

We do have some "Try it" sections with pros, e.g. https://developer.mozilla.org/en-US/docs/Web/CSS/max#try_it

In the mdn/content PR we can put the text below the macro call. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still trying to see if we can maintain this info in the example itself. How about using the text from your content PR:
display-interactive-2

or even a more shortened version:

display-interactive-4

Else the last option is to add text in content after the macro call.

Copy link
Contributor Author

@OnkarRuikar OnkarRuikar Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's pic the shortest version Apply different <code>display</code> values on the dashed orange-bordered <code>div</code>, which contains three child elements. bef7553

Some text A.
<div id="example-element">
<div class="child">One</div>
<div class="child">Two</div>
<div class="child">Three</div>
<div class="child">Child 1</div>
<div class="child">Child 2</div>
<div class="child">Child 3</div>
</div>
Some text B.
</div>
</section>
</div>