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.

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

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner January 10, 2024 07:45
@OnkarRuikar OnkarRuikar requested review from dipikabh and removed request for a team January 10, 2024 07:45
@dipikabh dipikabh self-assigned this Jan 11, 2024
@dipikabh
Copy link
Contributor

This is close. But I think the additional text in the example does not fully clarify the element on which display is being set, especially considering that the OP in the issue expected each child element to have its own display setting.

Brainstorming an idea here. How about we add a description outside the div to explain the example, something like "The orange element represents the div on which different display values are set. It contains three child elements." This can be followed by:

    <div class="example-container">
      some text A
      <div id="example-element">
        <div class="child">Child 1</div>
        <div class="child">Child 2</div>
        <div class="child">Child 3</div>
      </div>
      some text B
    </div>

With display: none, the current text in the proposed PR reads: "Change display property of this div element.". It refers to the div element that gets hidden when display: none. Instead of using a complete sentence, using placeholders like "some text A" and "some text B" could demonstrate the absence of the div more effectively. With display: none, the output would read "some text A some text B"", indicating that the div is not displayed.

Let me know what you think.

@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Jan 12, 2024

How about we add a description outside the div to explain the example, something like "The orange element represents the div on which different display values are set. It contains three child elements." This can be followed by:

@dipikabh Yes, we need to be more explicit.

@OnkarRuikar OnkarRuikar requested a review from estelle January 12, 2024 06:13
Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

LGTM. @dipikabh I'll let you merge both PRs at the same time if this PR is ok with you.

Comment on lines 38 to 40
<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

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

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

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

Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements, @OnkarRuikar!

@dipikabh dipikabh merged commit 2125dd1 into mdn:main Jan 16, 2024
5 checks passed
@OnkarRuikar OnkarRuikar deleted the css_display_live branch January 16, 2024 14:16
@OnkarRuikar
Copy link
Contributor Author

@dipikabh you'll have to merge #2716 as well to make it live.

@dipikabh
Copy link
Contributor

you'll have to merge #2716 as well to make it live.

Done 👍

@wbamberg
Copy link
Contributor

Thanks! FWIW I agree with @dipikabh that we should try very hard to keep interactive examples self-contained. And that really any kind of exposition in the examples is unfortunate, but sometimes as in this case it is unavoidable (as also for instance here: https://developer.mozilla.org/en-US/docs/Web/CSS/position). But I think what you have here is perfect :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong Visulation of Display Property
4 participants