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

New .admonition/.details injects padding into *any* last-child #630

Closed
facelessuser opened this Issue Dec 9, 2017 · 13 comments

Comments

Projects
None yet
3 participants
@facelessuser
Contributor

facelessuser commented Dec 9, 2017

Description

Currently Material is assigning any element that is a last-child with padding. This is bad. So the math elements have some children who or last-child to their parent even if they are not direct children. of .admonition. Since they are descendants, they inherit the padding.

Something like .admonition>:last-child and details>:last-child would be more appropriate. as it would limit the padding the last direct child.

Please see issue: facelessuser/pymdown-extensions#187.

Expected behavior

Padding should only be applied to the last direct descendant of .admonition or details.

Actual behavior

Any element that qualifies as a last-child is inheriting bottom padding.

Steps to reproduce the bug

  1. NA

Package versions

  • Material: 2.2.1 & 2.2.2

Project configuration

https://github.com/facelessuser/pymdown-extensions/blob/master/mkdocs.yml

System information

  • OS: NA
  • Browser: NA
@facelessuser

This comment has been minimized.

Show comment
Hide comment
@facelessuser

facelessuser Dec 9, 2017

Contributor

On a side note, I also question this case as it is also very aggressive, but I don't think I see any side effects of it, but maybe should be evaluated.

.md-typeset .admonition :first-child, .md-typeset details :first-child {
    margin-top: 0;
}
Contributor

facelessuser commented Dec 9, 2017

On a side note, I also question this case as it is also very aggressive, but I don't think I see any side effects of it, but maybe should be evaluated.

.md-typeset .admonition :first-child, .md-typeset details :first-child {
    margin-top: 0;
}
@facelessuser

This comment has been minimized.

Show comment
Hide comment
@facelessuser

facelessuser Dec 9, 2017

Contributor

I'll probably try and throw together a pull this week. I figure the first-child should probably be parent>:nth-child(2) and last-child should probably be parent>:last-child.

Contributor

facelessuser commented Dec 9, 2017

I'll probably try and throw together a pull this week. I figure the first-child should probably be parent>:nth-child(2) and last-child should probably be parent>:last-child.

@max-ci

This comment has been minimized.

Show comment
Hide comment
@max-ci

max-ci Dec 10, 2017

.md-typeset .admonition :first-child,
.md-typeset details :first-child {
  margin-top: 0;
}

Is this necessary at all?

max-ci commented Dec 10, 2017

.md-typeset .admonition :first-child,
.md-typeset details :first-child {
  margin-top: 0;
}

Is this necessary at all?

@facelessuser

This comment has been minimized.

Show comment
Hide comment
@facelessuser

facelessuser Dec 10, 2017

Contributor

@makshh, I assume it is there to limit some elements that add extra margin to their top initially. I suspect the idea is the first block element should not have extra padding in an admonition or details element to keep a consistent look.

I practicality, I'm not sure if there are specific elements in Material that are adding extra margins that need to be stripped. I assume it was added for a reason, but I don't know.

Contributor

facelessuser commented Dec 10, 2017

@makshh, I assume it is there to limit some elements that add extra margin to their top initially. I suspect the idea is the first block element should not have extra padding in an admonition or details element to keep a consistent look.

I practicality, I'm not sure if there are specific elements in Material that are adding extra margins that need to be stripped. I assume it was added for a reason, but I don't know.

@max-ci

This comment has been minimized.

Show comment
Hide comment
@max-ci

max-ci Dec 10, 2017

There is also a problem with specificity, this screenshot is from official GitHub page.

image

And regarding to your fixes, I think :nth-child(2) won't be good.
Example: admonition without title with two paragraphs (second paragraph will have margin-top of 0 unless it has higher specificity).

max-ci commented Dec 10, 2017

There is also a problem with specificity, this screenshot is from official GitHub page.

image

And regarding to your fixes, I think :nth-child(2) won't be good.
Example: admonition without title with two paragraphs (second paragraph will have margin-top of 0 unless it has higher specificity).

@facelessuser

This comment has been minimized.

Show comment
Hide comment
@facelessuser

facelessuser Dec 10, 2017

Contributor

Hmm, that leads me to believe that it is targeting the title element. If that is its real purpose, then that could be targeted instead with .admonition>.admonition-title and details>summary I know that details is kind of inheriting style from admonitions, so I wonder it was made more general to suit the both.

Contributor

facelessuser commented Dec 10, 2017

Hmm, that leads me to believe that it is targeting the title element. If that is its real purpose, then that could be targeted instead with .admonition>.admonition-title and details>summary I know that details is kind of inheriting style from admonitions, so I wonder it was made more general to suit the both.

@squidfunk

This comment has been minimized.

Show comment
Hide comment
@squidfunk

squidfunk Dec 12, 2017

Owner

Attempted a fix in PR #634, specifically in this file. The margin issue @makshh mentioned is also fixed. The problem was that, indeed, all margin was applied to all nested last childs. This should be fixed by the child selector. Just merged the PR into master - could you validate?

EDIT: this also applies for details, as it is derived from admonition.

Owner

squidfunk commented Dec 12, 2017

Attempted a fix in PR #634, specifically in this file. The margin issue @makshh mentioned is also fixed. The problem was that, indeed, all margin was applied to all nested last childs. This should be fixed by the child selector. Just merged the PR into master - could you validate?

EDIT: this also applies for details, as it is derived from admonition.

@facelessuser

This comment has been minimized.

Show comment
Hide comment
@facelessuser

facelessuser Dec 12, 2017

Contributor

Fantastic! Meant to get to this myself, but you beat me to it 🙂.

Contributor

facelessuser commented Dec 12, 2017

Fantastic! Meant to get to this myself, but you beat me to it 🙂.

@squidfunk

This comment has been minimized.

Show comment
Hide comment
@squidfunk

squidfunk Dec 12, 2017

Owner

Did you test it? Can I make a release with a fix? I lack further use cases :-)

Owner

squidfunk commented Dec 12, 2017

Did you test it? Can I make a release with a fix? I lack further use cases :-)

@facelessuser

This comment has been minimized.

Show comment
Hide comment
@facelessuser

facelessuser Dec 12, 2017

Contributor

I will formally test your changes and let you know, but I had essentially tested both of your changes directly in the browser when I was investigating.

Contributor

facelessuser commented Dec 12, 2017

I will formally test your changes and let you know, but I had essentially tested both of your changes directly in the browser when I was investigating.

@squidfunk squidfunk self-assigned this Dec 12, 2017

@squidfunk squidfunk added the bug label Dec 12, 2017

@max-ci

This comment has been minimized.

Show comment
Hide comment
@max-ci

max-ci Dec 12, 2017

I think it is fixed.

Possible cases when it may not work as expected:

  1. Big top margin on first child (for example if you include heading inside admonition it will have big top margin).

  2. Last child of the last element inside admonition has margin bottom (so the margin will be margin of that element + 1.2rem defined in the fix).

But I think it's acceptable as it is now, these are edge cases.

max-ci commented Dec 12, 2017

I think it is fixed.

Possible cases when it may not work as expected:

  1. Big top margin on first child (for example if you include heading inside admonition it will have big top margin).

  2. Last child of the last element inside admonition has margin bottom (so the margin will be margin of that element + 1.2rem defined in the fix).

But I think it's acceptable as it is now, these are edge cases.

@facelessuser

This comment has been minimized.

Show comment
Hide comment
@facelessuser

facelessuser Dec 13, 2017

Contributor

@squidfunk, my use cases are fixed. 👍

Contributor

facelessuser commented Dec 13, 2017

@squidfunk, my use cases are fixed. 👍

@squidfunk

This comment has been minimized.

Show comment
Hide comment
@squidfunk

squidfunk Dec 13, 2017

Owner

Thanks for your feedback! Released as part of 2.2.3.

Owner

squidfunk commented Dec 13, 2017

Thanks for your feedback! Released as part of 2.2.3.

@squidfunk squidfunk closed this Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment