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

CSS: "alert" is missing "margin-top" #1390

Closed
mgeier opened this issue Apr 26, 2016 · 4 comments · Fixed by #1396
Closed

CSS: "alert" is missing "margin-top" #1390

mgeier opened this issue Apr 26, 2016 · 4 comments · Fixed by #1396

Comments

@mgeier
Copy link
Contributor

mgeier commented Apr 26, 2016

Currently, the alert class has only margin-bottom (I guess this comes from bootstrap):

.alert {
    margin-bottom: 18px;
}

Since most other elements use only margin-top, those alert boxes are much too close to whatever comes before them.

Probably something like this could be added:

.rendered_html * + .alert {
    margin-top: 18px;
}

... or it could just be added unconditionally, like the original margin-bottom:

.alert {
    margin-top: 18px;
}

But probably the best solution would be, to overwrite the original margin-bottom and just do the same as with plain old <p>:

.rendered_html .alert {
    margin-bottom: initial;
}
.rendered_html * + .alert {
    margin-top: 1em;
}
@ellisonbg
Copy link
Contributor

Yeah, I have seen this before and we should fix it. Can you add some
screenshots to help design review?

On Tue, Apr 26, 2016 at 2:21 AM, Matthias Geier notifications@github.com
wrote:

Currently, the alert class has only margin-bottom (I guess this comes
from bootstrap):

.alert {
margin-bottom: 18px;
}

Since most other elements use only margin-top, those alert boxes are much
too close to whatever comes before them.

Probably something like this could be added:

.rendered_html * + .alert {
margin-top: 18px;
}

... or it could just be added unconditionally, like the original
margin-bottom:

.alert {
margin-top: 18px;
}

But probably the best solution would be, to overwrite the original
margin-bottom and just do the same as with plain old

:

.rendered_html .alert {
margin-bottom: initial;
}.rendered_html * + .alert {
margin-top: 1em;
}


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1390

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@mgeier
Copy link
Contributor Author

mgeier commented Apr 27, 2016

Sure, why not.

before:

before

after:

after

mgeier added a commit to mgeier/notebook that referenced this issue Apr 27, 2016
mgeier added a commit to mgeier/notebook that referenced this issue Apr 27, 2016
@mgeier
Copy link
Contributor Author

mgeier commented Apr 27, 2016

I've created a PR: #1396.
I hope I added it in the right place ...

@willingc
Copy link
Member

Thanks for the PR @mgeier and for improving the visual output. 🍰

@minrk minrk added this to the 5.0 milestone May 30, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants