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

Right align sage/python tabs and alter vertical spacing #38568

Merged
merged 11 commits into from
Sep 15, 2024

Conversation

thecaligarmo
Copy link
Contributor

@thecaligarmo thecaligarmo commented Aug 27, 2024

Fixes #37760

The css will push the tabs all the way to the right and alter the vertical spacing so that they are more "in-line". The vertical spacing causes some issues as sometimes the text from the previous line will run into the tabs. To fix this, the previous line now has a max-width of 80% so that they won't overlap. This also makes it so that the EXAMPLES: line is on the same line as tabs. (As far as I could tell, there was no way to distinguish the EXAMPLES: text from other text).

A good page to see the changes is on:
en/reference/rings_standard/sage/rings/integer_ring.html
as it contains various different formats for sage/python examples.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@mantepse
Copy link
Collaborator

Here is an alternative suggestion (I don't know anything about web programming and only little about user interfaces, so feel free to ignore).

Many web pages have a global language switch, often indicated by a flag of the corresponding country, in the top right (https://ux.stackexchange.com/questions/15041/placement-of-a-change-language-button)

Since the code shown is also globally changed, maybe this would be a good approach.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 27, 2024

Many web pages have a global language switch, often indicated by a flag of the corresponding country, in the top right

Well I thought about this idea when I made the original design, but there are several reasons why I decided to use tabs. The main one:

  • Because the preparser is run to create the pure Python examples, they are often not very readable. So even people who want to copy-paste pure Python examples into a terminal or notebook may actually want to read the original Sage example and then switch the tab just to copy-paste.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 27, 2024

This also makes it so that the EXAMPLES: line is on the same line as tabs. (As far as I could tell, there was no way to distinguish the EXAMPLES: text from other text).

I think @kwankyu's #37614 could be adapted for this

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 27, 2024

I recommend to merge #38468 so that the "Build documentation" workflow runs through

@thecaligarmo
Copy link
Contributor Author

I've merged #38468 as requested. I've also adapted #37614 in order to add a little bit of space below EXAMPLE texts.

Copy link

github-actions bot commented Aug 27, 2024

Documentation preview for this PR (built with commit 66e01fa; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 27, 2024

Beautiful. Thanks @thecaligarmo for working on this!

Note that in https://doc-release--sagemath.netlify.app/html/en/reference/tensor_free_modules/sage/tensor/modules/finite_rank_free_module we use a third tab "Sage Live", so more space will be needed

@thecaligarmo
Copy link
Contributor Author

Ok, so I changed a little bit how it works so that we can do variable widths depending on how many tabs there are. It should make it so it looks nice regardless of number of tabs.

@mkoeppe mkoeppe requested a review from kwankyu August 28, 2024 01:33
@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2024

Screen Shot 2024-08-28 at 12 20 14 PM

As seen above, I am not sure if this improves the situation. For the three tabs, the tab labels will create even more space to the right. i doubt that moving tab labels to the right would only improve the situation. It creates other problem.

What do we want to solve? Perhaps we want to move the tab labels completely out of sight. If not completely, then we may just shrink the spaces that the tab labels occupy (without the "kerning"). Or there may be other creative solutions. I am not sure what would be the best solution at the moment...

@thecaligarmo
Copy link
Contributor Author

I think, personally, I would generally prefer the tabs on the left. By putting it to the right it obfuscates it a little and it's harder to know they're tabs to begin with. The documentation is a little bulkier with the tabs on the left, but it makes it nicer from a UX perspective.

We can shrink the gap between the tabs and the previous sentence to make it a little smaller, but I don't think it's that big of an issue?

An alternative design would be to put the tabs completely to the left of the code boxes. The problem with that would be that we have less room for the code boxes. This theoretically shouldn't be an issue as code length should be less than 80 chars, but that's not always the case.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2024

... If not completely, then we may just shrink the spaces that the tab labels occupy (without the "kerning").

Screen Shot 2024-08-28 at 1 13 11 PM

or even

Screen Shot 2024-08-28 at 1 15 53 PM

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2024

I think, personally, I would generally prefer the tabs on the left. By putting it to the right it obfuscates it a little and it's harder to know they're tabs to begin with. The documentation is a little bulkier with the tabs on the left, but it makes it nicer from a UX perspective.

I add that psychologically we don't skip the tab labels on the right, we tend to look at them, scanning all the way to the right.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 28, 2024

I add that psychologically we don't skip the tab labels on the right, we tend to look at them, scanning all the way to the right.

I think adding a bit of vertical spacing above the tabs might eliminate this effect.

@AndrewMathas
Copy link
Member

I think, personally, I would generally prefer the tabs on the left. By putting it to the right it obfuscates it a little and it's harder to know they're tabs to begin with. The documentation is a little bulkier with the tabs on the left, but it makes it nicer from a UX perspective.

I also prefer the tabs on the left. I don't see much difference with the spacing tweaks to the labels.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2024

I also prefer the tabs on the left. I don't see much difference with the spacing tweaks to the labels.

How about dimming the labels (the last screenshot above)? No much difference?

@thecaligarmo
Copy link
Contributor Author

... If not completely, then we may just shrink the spaces that the tab labels occupy (without the "kerning").

Screen Shot 2024-08-28 at 1 13 11 PM

or even

Screen Shot 2024-08-28 at 1 15 53 PM

These look good. I can try and add this tomorrow when I wake up. I'll also put it back on the left unless we're set on it being put on the right?

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2024

Good for me. Thanks.

@AndrewMathas
Copy link
Member

Dimming the labels is fine, although I think it better to highlight the current tab, as in:
Screenshot 2024-08-28 at 14 58 36

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 28, 2024

Yes, in isolation. But we see too many of them (tabbed examples). I prefer just highlighting the underline (bottom border line).

We may choose one if the author, hopefully, prepares a few design candidates :-)

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 4, 2024

Thanks for the work!

Copy link
Member

@AndrewMathas AndrewMathas 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 to me.

@thecaligarmo
Copy link
Contributor Author

Sounds good. I'll update the code to reflect the chosen design and push when it's ready (hopefully in the next day or two). I'll update the ticket when the design is updated

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 4, 2024

I thought the PR branch is already for (AL). No?

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 4, 2024

Anyway I will remove the label to give you enough time to update the code.

@thecaligarmo
Copy link
Contributor Author

I thought the PR branch is already for (AL). No?

No, it's currently still on the right hand side. I stopped editing it while we were voting on it. It shouldn't take to long to do, but I'm away from my computer until Thursday night/Friday, so it should be finished by the end of this week.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 4, 2024

The doc preview shows the tabs on the left...

@thecaligarmo
Copy link
Contributor Author

Yes. For the previews I made local changes that didn't affect the branch itself. This was done because there were many options and it didn't seem viable (and would be a waste of time) to make a commit for every single option. So none of those are in the branch.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 5, 2024

I meant #38568 (comment).

Anyway, take your time.

@thecaligarmo
Copy link
Contributor Author

@kwankyu Ah ok! I had missed my comment that I had actually updated my branch to include (AL), so I was wrong. Sorry about that. The thing that I hadn't finished doing was cleaning up the code of unnecessary things. I've gone ahead and done that and it should now be ready.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 5, 2024

Otherwise lgtm.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
…cing

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37760

The css will push the tabs all the way to the right and alter the
vertical spacing so that they are more "in-line".  The vertical spacing
causes some issues as sometimes the text from the previous line will run
into the tabs. To fix this, the previous line now has a `max-width` of
`80%` so that they won't overlap.  This also makes it so that the
`EXAMPLES:` line is on the same line as tabs. (As far as I could tell,
there was no way to distinguish the `EXAMPLES:` text from other text).

A good page to see the changes is on:
`en/reference/rings_standard/sage/rings/integer_ring.html`
as it contains various different formats for sage/python examples.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
- Depends on sagemath#38468 (merged here)
    
URL: sagemath#38568
Reported by: Aram Dermenjian
Reviewer(s): Andrew Mathas, Aram Dermenjian, Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
…cing

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37760

The css will push the tabs all the way to the right and alter the
vertical spacing so that they are more "in-line".  The vertical spacing
causes some issues as sometimes the text from the previous line will run
into the tabs. To fix this, the previous line now has a `max-width` of
`80%` so that they won't overlap.  This also makes it so that the
`EXAMPLES:` line is on the same line as tabs. (As far as I could tell,
there was no way to distinguish the `EXAMPLES:` text from other text).

A good page to see the changes is on:
`en/reference/rings_standard/sage/rings/integer_ring.html`
as it contains various different formats for sage/python examples.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
- Depends on sagemath#38468 (merged here)
    
URL: sagemath#38568
Reported by: Aram Dermenjian
Reviewer(s): Andrew Mathas, Aram Dermenjian, Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 10, 2024
…cing

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37760

The css will push the tabs all the way to the right and alter the
vertical spacing so that they are more "in-line".  The vertical spacing
causes some issues as sometimes the text from the previous line will run
into the tabs. To fix this, the previous line now has a `max-width` of
`80%` so that they won't overlap.  This also makes it so that the
`EXAMPLES:` line is on the same line as tabs. (As far as I could tell,
there was no way to distinguish the `EXAMPLES:` text from other text).

A good page to see the changes is on:
`en/reference/rings_standard/sage/rings/integer_ring.html`
as it contains various different formats for sage/python examples.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
- Depends on sagemath#38468 (merged here)
    
URL: sagemath#38568
Reported by: Aram Dermenjian
Reviewer(s): Andrew Mathas, Aram Dermenjian, Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 12, 2024
…cing

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37760

The css will push the tabs all the way to the right and alter the
vertical spacing so that they are more "in-line".  The vertical spacing
causes some issues as sometimes the text from the previous line will run
into the tabs. To fix this, the previous line now has a `max-width` of
`80%` so that they won't overlap.  This also makes it so that the
`EXAMPLES:` line is on the same line as tabs. (As far as I could tell,
there was no way to distinguish the `EXAMPLES:` text from other text).

A good page to see the changes is on:
`en/reference/rings_standard/sage/rings/integer_ring.html`
as it contains various different formats for sage/python examples.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
- Depends on sagemath#38468 (merged here)
    
URL: sagemath#38568
Reported by: Aram Dermenjian
Reviewer(s): Andrew Mathas, Aram Dermenjian, Kwankyu Lee, Matthias Köppe
@vbraun vbraun merged commit 468d3ee into sagemath:develop Sep 15, 2024
15 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTML documentation: Right-align Sage / Python inline tabs, use vertical kerning
8 participants