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

Minimum top margin in tabbed examples #820

Merged
merged 5 commits into from
Sep 29, 2022

Conversation

NiedziolkaMichal
Copy link
Member

@NiedziolkaMichal NiedziolkaMichal commented Jun 20, 2022

This PR is a fix for #743 and #817.
Problem is that Output Label partically hides the content of examples that don't have "margin-top" property set, on the first element present in example.
When first element is label, table, img or any other element without margin-top, example looks like this:
image
When margin-top is set, like in element p, example looks fine:
image

I discussed with wbamberg in #743 to add padding-top to "output", which would move downwards every example, or to remove label altogether. Today I thought to add:

<div class="output-top-margin"></div>

at the very top of "output" element and to create following rule:

.output-top-margin {
  margin-bottom: 10px;
}

This would take advantage of margin collapsing, so p examples would looks exactly the same as they are now, while examples without margin on the top would be moved a bit lower. In this PR I have changed 10px(default margin to p) to 13px, because examples look better that way:
image

image

@queengooborg
Copy link
Collaborator

I personally think that a simple padding-top: 2rem; is fine. I don't think that there will be many cases where we'll want to collapse the margins; I can only think of ones where margins are added to move the example below the label already, which we should remove anyways. This would also result in simpler code and make our intent slightly clearer.

Would you be down for that?

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@queengooborg queengooborg linked an issue Sep 13, 2022 that may be closed by this pull request
@NiedziolkaMichal
Copy link
Member Author

@queengooborg I have checked examples and indeed padding-top: 2rem; would be fine too. There are 2 interactive examples which will create a scrollbar because of this change, but it's insignificant.

Copy link
Collaborator

@queengooborg queengooborg 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 this, LGTM!

@queengooborg queengooborg merged commit 870de64 into mdn:main Sep 29, 2022
NiedziolkaMichal added a commit to NiedziolkaMichal/bob that referenced this pull request Sep 29, 2022
queengooborg pushed a commit that referenced this pull request Sep 30, 2022
* Tabbed examples use IFrame instead of Shadow DOM

* Applied padding change from #820
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.

OUTPUT label hiding content
2 participants