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

docs: Refactor and add test for paginated_return_data.py #3456

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kedod
Copy link
Contributor

@kedod kedod commented Apr 30, 2024

Description

  • Refactor and add test for paginated_return_data doc example

Visible code shows the simpliest usage of the funcionality.
The Full code dropdown shows the full working example code with highlighted lines.

image
image

Closes

@kedod kedod requested review from a team as code owners April 30, 2024 18:53
@github-actions github-actions bot added area/docs This PR involves changes to the documentation size: small pr/external Triage Required 🏥 This requires triage labels Apr 30, 2024
@kedod kedod force-pushed the add-test-for-paginated-return-dto branch 3 times, most recently from 47c57c0 to 652c43f Compare April 30, 2024 19:02
@kedod kedod force-pushed the add-test-for-paginated-return-dto branch from 79327b2 to 652c43f Compare April 30, 2024 19:28
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3456

Copy link
Contributor

@peterschutt peterschutt left a comment

Choose a reason for hiding this comment

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

Thanks! I think the auto format might have thrown out the line numbers by one.

Do you think we should we use literal includes for the remaining code blocks in that section?

docs/usage/dto/1-abstract-dto.rst Outdated Show resolved Hide resolved
docs/usage/dto/1-abstract-dto.rst Outdated Show resolved Hide resolved
docs/usage/dto/1-abstract-dto.rst Outdated Show resolved Hide resolved
@JacobCoffee
Copy link
Member

JacobCoffee commented Apr 30, 2024

im struggling with this because now we split into two places and increase maintenance burden around :linenos: upkeep but on the flip side I really hate scrolling through giant walls of code to get to the next section.

Aesthetically I would prefer the usage of sphinx-togglebutton. I think it looks a lot cleaner and is customizable. (https://shibuya.lepture.com/extensions/sphinx-togglebutton/)

Maybe we could standardize on just always showing full blocks of code, but if those are over a certain line count e.g., wont fit in half of the viewport or maybe the full length of the viewport then we chunk it into a .. dropdown::`` directive with :class: dropdown` to style it?

@Alc-Alc Alc-Alc changed the title Refactor and add test for paginated_return_data.py docs: Refactor and add test for paginated_return_data.py May 5, 2024
@kedod
Copy link
Contributor Author

kedod commented May 5, 2024

im struggling with this because now we split into two places and increase maintenance burden around :linenos: upkeep but on the flip side I really hate scrolling through giant walls of code to get to the next section.

Aesthetically I would prefer the usage of sphinx-togglebutton. I think it looks a lot cleaner and is customizable. (https://shibuya.lepture.com/extensions/sphinx-togglebutton/)

Maybe we could standardize on just always showing full blocks of code, but if those are over a certain line count e.g., wont fit in half of the viewport or maybe the full length of the viewport then we chunk it into a .. dropdown:: directive with ``:class: dropdown` to style it?

That's ok but I'm afraid that after examples rewrite (to make them self contained) the most of them will be too long to make them visible at start and "unhiding" every example can be user unfriendly too.
I have one more idea.
Custom extension

Let's say:

.. dropdownliteralinclude:: /examples/data_transfer_objects/factory/paginated_return_data.py
    :language: python
    :lines: 9-11,26-40

It can work similar to Slack. Take a look:
image

We can show only lines at start then highlight the same lines after expanding.
What do you think about this @JacobCoffee ?
For huge files we can still use togglebutton you suggested.

If you like this idea I can look for an extension or try to implement it on my own.

@kedod kedod force-pushed the add-test-for-paginated-return-dto branch from c7e8958 to 631051c Compare May 5, 2024 15:07
Copy link

sonarqubecloud bot commented May 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@provinzkraut
Copy link
Member

im struggling with this because now we split into two places and increase maintenance burden around :linenos: upkeep but on the flip side I really hate scrolling through giant walls of code to get to the next section.

Aesthetically I would prefer the usage of sphinx-togglebutton. I think it looks a lot cleaner and is customizable. (https://shibuya.lepture.com/extensions/sphinx-togglebutton/)

Maybe we could standardize on just always showing full blocks of code, but if those are over a certain line count e.g., wont fit in half of the viewport or maybe the full length of the viewport then we chunk it into a .. dropdown:: directive with ``:class: dropdown` to style it?

I have a quite strong opinions on the dropdowns in that I think they should almost always be avoided in reference docs when they're hiding code. The code is what these docs are talking about, so hiding it is simply obscuring one of the main pieces of information.

@JacobCoffee If you don't like scrolling, we should improve the section layout / side nav. But hiding code is the worst option IMO.

@cofin cofin force-pushed the add-test-for-paginated-return-dto branch from 631051c to d7293fe Compare August 20, 2024 20:14
@cofin cofin enabled auto-merge (squash) August 20, 2024 20:15
@cofin cofin force-pushed the add-test-for-paginated-return-dto branch from d7293fe to 3d2e572 Compare August 25, 2024 01:12
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs This PR involves changes to the documentation pr/external pr/internal size: small Triage Required 🏥 This requires triage type/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants