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

optimize figure bottom margin #1749

Merged
merged 3 commits into from
Apr 11, 2024
Merged

optimize figure bottom margin #1749

merged 3 commits into from
Apr 11, 2024

Conversation

berlin2123
Copy link
Contributor

Avoid multiple margin-bottom by p and figure, that result in too wide a bottom margin.

Avoid multiple margin-bottom by `p` and `figure`, that result in too wide a bottom margin.
@trallard
Copy link
Collaborator

trallard commented Apr 4, 2024

Thanks for the contribution @berlin2123, since there is no associated issue with this PR could you please add a screenshot of the before and after this change so we understand the rationale behind it?

@trallard trallard added the tag: CSS CSS and SCSS related issues label Apr 4, 2024
@berlin2123
Copy link
Contributor Author

before:

5(6G_$L5D~2BTVR$4JV@KR4

after:

_WF9TF%LGK%`}B%DZ(14_NG

@drammock
Copy link
Collaborator

drammock commented Apr 9, 2024

seems like a reasonable change to make. Linter is failing; @berlin2123 can you try installing our pre-commit hook? It should auto-fix linter errors at commit time.

@trallard
Copy link
Collaborator

@gabalafou This seems reasonable, but a last check would be helpful. If you are happy with this and so is the CI, you can go ahead and merge.

@gabalafou
Copy link
Collaborator

I pushed a few tweaks:

  • I added an example to Images and Figures that demonstrates the changes in this PR
  • I moved the margin-top adjustment to the figcaption because I think that's where it more properly belongs. I believe the intent is to just add some whitespace between the image and the caption, not add more whitespace between every paragraph in the caption
  • Similarly I moved the margin-bottom adjustment to the last paragraph in the figcaption, since I don't think the intent was to reduce the margin between paragraphs but only to prevent the last paragraph's bottom margin from adding on to the figcaption's bottom margin

@gabalafou
Copy link
Collaborator

gabalafou commented Apr 10, 2024

Before (figure example section)

pst-figures-example-before

After (figure example section)

pst-figures-example-after

@gabalafou
Copy link
Collaborator

@drammock could you give this another round of review?

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

kitchen sink docs pages should not be modified; they are vendored from an external source and periodically updated, so these changes will eventually get clobbered. See docs/community/topics/kitchen-sink.md and docs/scripts/update-kitchen-sink.py

Please either remove the caption alignment example, or move it to a file outside of docs/examples/kitchen-sink/

@gabalafou
Copy link
Collaborator

@drammock, ah, good to know. I decided to remove the example because it's essentially already covered at the bottom of Kitchen Sink / Structural Elements.

That said, I think the figure section should probably also have an example like the one on the structural elements page. Would it be possible to make this change upstream?

@drammock
Copy link
Collaborator

Would it be possible to make this change upstream?

you'd have to propose the change at https://github.com/sphinx-themes/sphinx-themes.org

If you clearly explain the rationale for the change (as you typically do 👍🏻) I'm confident that @pradyunsg would merge it

@drammock drammock merged commit 733d9f3 into pydata:main Apr 11, 2024
19 checks passed
gabalafou added a commit to gabalafou/pydata-sphinx-theme that referenced this pull request Apr 29, 2024
* figure bottom margin

Avoid multiple margin-bottom by `p` and `figure`, that result in too wide a bottom margin.

* Update _figures.scss

* Only adjust margins at top and bottom of figcaption

---------

Co-authored-by: gabalafou <[email protected]>
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
* figure bottom margin

Avoid multiple margin-bottom by `p` and `figure`, that result in too wide a bottom margin.

* Update _figures.scss

* Only adjust margins at top and bottom of figcaption

---------

Co-authored-by: gabalafou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: CSS CSS and SCSS related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants