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

Combine the Oblique Mercator projection pages into one page #3451

Merged
merged 10 commits into from
Sep 28, 2024

Conversation

yvonnefroehlich
Copy link
Member

@yvonnefroehlich yvonnefroehlich commented Sep 24, 2024

Description of proposed changes

This PR aims to combine the three documentation pages for the Oblique Mercator projection (Oa, Ob, Oc) into one common page. For the idea / context see #3407 (comment)

Preveiw:

Fixes #

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 wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.
  • Use underscores (not hyphens) in names of Python files and directories.

Slash Commands

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

  • /format: automatically format and lint the code

doc/techref/projections.md Show resolved Hide resolved
doc/techref/projections.md Show resolved Hide resolved
@yvonnefroehlich yvonnefroehlich changed the title POC: Combine the Mercator oblique projection pages into one page POC: Combine the Oblique Mercator projection pages into one page Sep 24, 2024
The projection pole is set by *lonp/latp*.
"""

# %%
Copy link
Member

Choose a reason for hiding this comment

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

I feel it's a little weird to show three mini maps in one gallery thumbnail image (https://pygmt-dev--3451.org.readthedocs.build/en/3451/projections/index.html#cylindric-projections). Since it's just three different ways to specify one projection, maybe we can show three separate maps in this example and only show one of them in the gallery summary page?

Copy link
Member Author

@yvonnefroehlich yvonnefroehlich Sep 26, 2024

Choose a reason for hiding this comment

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

I combined the three maps into one figure, because the maps for the options Oa and Ob are quite high. And having all three maps in the current size below each other makes the website quite long. But your are right, the thumbnail image looks not optimal.
In commit cd15b7d I tested showing separate maps for using a much smaler sizes for the maps. For the thumbnail image I picked the map for Oc, as the ascept ratio seems to fit best to the thumbnail image card.

Copy link
Member

Choose a reason for hiding this comment

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

And having all three maps in the current size below each other makes the website quite long.

I prefer to have the figure below each subsection, which is easier to read IMHO. Let's hear what others think about it.

Copy link
Member Author

@yvonnefroehlich yvonnefroehlich Sep 27, 2024

Choose a reason for hiding this comment

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

Ah, I have missed this point from your first review comment here. I just worked on removing the subplot. If having separate code blocks within each subsection, I think it's also OK to show the maps separatly, even though the maps have a larger height. However, having them all tree below each other at the end looked not good.

Togther at the end Separatly within each subsection
omp_at_the_end omp_within_each_subsection

Copy link
Member

Choose a reason for hiding this comment

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

I like the new layout.

@yvonnefroehlich yvonnefroehlich changed the title POC: Combine the Oblique Mercator projection pages into one page Combine the Oblique Mercator projection pages into one page Sep 26, 2024
@seisman seisman added the documentation Improvements or additions to documentation label Sep 27, 2024
@seisman seisman added this to the 0.14.0 milestone Sep 27, 2024
@seisman seisman added the needs review This PR has higher priority and needs review. label Sep 27, 2024
@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Sep 27, 2024
@yvonnefroehlich yvonnefroehlich self-assigned this Sep 27, 2024
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 28, 2024
@seisman seisman merged commit 780f164 into main Sep 28, 2024
11 checks passed
@seisman seisman deleted the combine-mercator-oblique branch September 28, 2024 07:07
@yvonnefroehlich
Copy link
Member Author

Do we also want to combine the Cartesian projection / transformation (linear, logarithmic, and exponential) into one page?

@seisman
Copy link
Member

seisman commented Sep 29, 2024

Do we also want to combine the Cartesian projection / transformation (linear, logarithmic, and exponential) into one page?

I prefer separate pages for them so that we can show all of them in the gallery page.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants