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

Increase minimum thumbnail height in style.css to fit text #1575

Merged
merged 4 commits into from
Oct 22, 2021

Conversation

maxrjones
Copy link
Member

Description of proposed changes

Sphinx-gallery updated the default thumbnail size in v0.9.0, such that the text does not fit in our boxes in the devdocs gallery. This PR increases the height slightly for the gallery and the team page so the text fits.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@maxrjones maxrjones added documentation Improvements or additions to documentation maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog labels Oct 5, 2021
@willschlitzer willschlitzer mentioned this pull request Oct 7, 2021
35 tasks
@seisman seisman added this to the 0.5.0 milestone Oct 9, 2021
@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Oct 9, 2021
@@ -209,6 +209,11 @@ a.copybtn {
text-align: center!important;
}

.sphx-glr-thumbcontainer {
min-height: 240px !important;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to increase this a bit more? This is what I see on Firefox:

image

@maxrjones
Copy link
Member Author

Does anyone have any ideas for what might be going on with the vercel build failure?

@weiji14
Copy link
Member

weiji14 commented Oct 12, 2021

Does anyone have any ideas for what might be going on with the vercel build failure?

Appears to be a problem with pip-21.3 released 21 hours ago, see pypa/pip#10573. Will need to wait for bugfix at pypa/pip#10577 and pip-21.3.1 release.

@maxrjones
Copy link
Member Author

Does anyone have any ideas for what might be going on with the vercel build failure?

Appears to be a problem with pip-21.3 released 21 hours ago, see pypa/pip#10573.

Ah, thanks. What do you think about pinning setuptools and pip until the issue is closed? I would prefer this to prevent people from having difficulties with their local dev environments.

@weiji14
Copy link
Member

weiji14 commented Oct 12, 2021

Does anyone have any ideas for what might be going on with the vercel build failure?

Appears to be a problem with pip-21.3 released 21 hours ago, see pypa/pip#10573.

Ah, thanks. What do you think about pinning setuptools and pip until the issue is closed? I would prefer this to prevent people from having difficulties with their local dev environments.

I think the pip issue might resolve itself in 24 hours, so probably don't need to do the pinning/unpinning dance. It's a little annoying since it makes reviewing this visual PR harder, but not in a rush since we're pushing out the PyGMT v0.5.0 release date.

@maxrjones
Copy link
Member Author

Does anyone have any ideas for what might be going on with the vercel build failure?

Appears to be a problem with pip-21.3 released 21 hours ago, see pypa/pip#10573.

Ah, thanks. What do you think about pinning setuptools and pip until the issue is closed? I would prefer this to prevent people from having difficulties with their local dev environments.

I think the pip issue might resolve itself in 24 hours, so probably don't need to do the pinning/unpinning dance. It's a little annoying since it makes reviewing this visual PR harder, but not in a rush since we're pushing out the PyGMT v0.5.0 release date.

OK. I actually do not even know how this change looks, since I ran into the same problem. I will simply wait before working more on the PRs.

@maxrjones
Copy link
Member Author

@weiji14, was this an accidental close?

@weiji14 weiji14 reopened this Oct 21, 2021
@weiji14
Copy link
Member

weiji14 commented Oct 21, 2021

Oops, sorry. I've reopened the PR.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Ok, the height looks long enough now. Feel like the width could be made a little wider since some of the maps are touching the border of the cards, but I think it's ok-ish:

image

Copy link
Member

@michaelgrund michaelgrund left a comment

Choose a reason for hiding this comment

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

Looks good!

@maxrjones
Copy link
Member Author

Ok, the height looks long enough now. Feel like the width could be made a little wider since some of the maps are touching the border of the cards, but I think it's ok-ish:

I'd prefer to get this right now, but do not want to commit any changes after approval without discussion first. Widening the thumbnails wouldn't affect the distance between the image and the border since they are scaled. We could instead add a padding to the image:

.sphx-glr-thumbcontainer img {
    max-width:95%!important;
}

image

Also, if you think there's too much whitespace around the text we could adjust the width in addition to the height instead of just the height to accommodate the text description:

.sphx-glr-thumbcontainer {
    min-height: 240px!important;
    min-width: 180px!important;
}

.sphx-glr-thumbcontainer img {
    max-width:95%!important;
}

image

@weiji14
Copy link
Member

weiji14 commented Oct 21, 2021

Ok, the height looks long enough now. Feel like the width could be made a little wider since some of the maps are touching the border of the cards, but I think it's ok-ish:

I'd prefer to get this right now, but do not want to commit any changes after approval without discussion first. Widening the thumbnails wouldn't affect the distance between the image and the border since they are scaled. We could instead add a padding to the image:

.sphx-glr-thumbcontainer img {
    max-width:95%!important;
}

Also, if you think there's too much whitespace around the text we could adjust the width in addition to the height instead of just the height to accommodate the text description:

.sphx-glr-thumbcontainer {
    min-height: 240px!important;
    min-width: 180px!important;
}

.sphx-glr-thumbcontainer img {
    max-width:95%!important;
}

I like the second option (adjust both the width of the thumbnail image and the text below it). Since the text doesn't wrap to newlines so much, we could even keep the height at 240px (which is what you did I think?).

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

🚀

@maxrjones maxrjones merged commit 8051339 into main Oct 22, 2021
@maxrjones maxrjones deleted the css-thumbnail-size branch October 22, 2021 22:31
@maxrjones maxrjones removed the final review call This PR requires final review and approval from a second reviewer label Feb 21, 2022
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…ppingTools#1575)

* Increase min thumbnail height to fit text

* Add max-width for image and min-width for container

Co-authored-by: Wei Ji <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants