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

Resize landing page logos for better display #3243

Closed

Conversation

haticekaratay
Copy link
Contributor

Description

This pull request is to adjust the size of the logos on the landing page to improve appearance and ensure better visual alignment. Previously they were too large.
Before:
image
After: The rampviz logo in the video is a placeholder to demonstrate better for the outcome.

Screen.Recording.2024-10-22.at.12.54.28.PM.mov

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@github-actions github-actions bot added the documentation Explanation of code and concepts label Oct 22, 2024
@pllim pllim added the no-changelog-entry-needed changelog bot directive label Oct 22, 2024
@pllim pllim added this to the 4.0.1 milestone Oct 22, 2024
@pllim pllim added the 💤backport-v4.0.x on-merge: backport to v4.0.x label Oct 22, 2024
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@kecnry
Copy link
Member

kecnry commented Oct 22, 2024

Do we understand how this "broke" before (was it an update to RTD styles or something)? Are all of the CSS changes here necessary? I'm just a bit worried about unintended consequences to changing global styling - but nothing stands out as being a problem in the rendered docs, so maybe its ok?

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.63%. Comparing base (f5b3a1d) to head (b604dd8).
Report is 108 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3243   +/-   ##
=======================================
  Coverage   88.62%   88.63%           
=======================================
  Files         125      125           
  Lines       18775    18779    +4     
=======================================
+ Hits        16640    16644    +4     
  Misses       2135     2135           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@haticekaratay
Copy link
Contributor Author

Do we understand how this "broke" before (was it an update to RTD styles or something)? Are all of the CSS changes here necessary? I'm just a bit worried about unintended consequences to changing global styling - but nothing stands out as being a problem in the rendered docs, so maybe its ok?

I checked all the other pages locally and found that it didn't affect them. Only CSS changes were made to scale the logo by 70% and to add a margin between the button and the logo.

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines -18 to -23
.sd-card .sd-card-img-top {
height: 52px;
width: 52px;
margin-left: auto;
margin-right: auto;
}
Copy link
Member

Choose a reason for hiding this comment

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

looks like :img-top is no longer used anywhere ✅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@pllim
Copy link
Contributor

pllim commented Oct 22, 2024

Do we understand how this "broke" before

Hmm yeah, they looked completely different and even compatible with dark mode until v4.0 when rampviz was also added.

https://jdaviz.readthedocs.io/en/v3.9.1/

Screenshot 2024-10-22 133210

@pllim
Copy link
Contributor

pllim commented Oct 22, 2024

pydata-sphinx-theme jump was only from 0.15.2 to 0.15.4 between those 2 releases.

@kecnry
Copy link
Member

kecnry commented Oct 22, 2024

hmmm did something else change in the style or index page that broke the layout and background color on the image tiles?

@pllim
Copy link
Contributor

pllim commented Oct 22, 2024

Hmm I don't see any CSS changes in v3.9.1...v4.0.0

@kecnry
Copy link
Member

kecnry commented Oct 22, 2024

Can we downgrade pydata-sphinx-theme to confirm that was the cause? We can also consider the remaining dark mode changes as a follow-up ticket since they are not introduced by this PR, and this PR greatly improve the layout!

@pllim
Copy link
Contributor

pllim commented Oct 22, 2024

Downgrade (#3244) didn't help.

https://jdaviz--3244.org.readthedocs.build/en/3244/

@kecnry
Copy link
Member

kecnry commented Oct 22, 2024

Thanks for checking! I'd vote for merging this as-is and creating a follow-up ticket to track down and fix the dark support then.

@pllim
Copy link
Contributor

pllim commented Oct 22, 2024

Wait

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I have alt fix

@pllim
Copy link
Contributor

pllim commented Oct 22, 2024

I was comparing with what astropy did and might have alternate fix. Please hold.

@pllim pllim mentioned this pull request Oct 22, 2024
9 tasks
@kecnry
Copy link
Member

kecnry commented Oct 22, 2024

are the changes to width and removing img-top worth putting on top of #3245?

@haticekaratay
Copy link
Contributor Author

haticekaratay commented Oct 22, 2024

Wow, there's been a lot happening since I last checked the chat. :) Maybe downgrading the sphinx-design or pydata-sphinx-theme could be the solution, as I didn't notice any CSS changes in the file itself. Instead of pinning the dependencies, should we focus on making our docs compatible with the latest versions? I'll test it out and report back here.

@pllim
Copy link
Contributor

pllim commented Oct 22, 2024

What about https://jdaviz--3245.org.readthedocs.build/en/3245/ over at #3245 ?

Sorry, I should have opened a PR here back when astropy was patched. The discussions here reminded me of that patch, so thanks!

@pllim
Copy link
Contributor

pllim commented Oct 22, 2024

worth putting on top

No, sorry. This PR is unnecessary with the other PR changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Explanation of code and concepts no-changelog-entry-needed changelog bot directive 💤backport-v4.0.x on-merge: backport to v4.0.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants