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

style(paragraph): add documentation for "scroll"'s "offset" #355

Merged
merged 2 commits into from
Aug 19, 2023

Conversation

hasezoey
Copy link
Contributor

This PR adds some rustdoc documentation and type helper to clarify what the values in Paragraph's scroll are

the types are useful because they are shown in rust-analyzer (though i can remove it if wanted)

Why?
because before it was not clear in what order the values should be

@codecov
Copy link

codecov bot commented Jul 30, 2023

Codecov Report

Merging #355 (07001ed) into main (440f62f) will increase coverage by 1.37%.
Report is 35 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #355      +/-   ##
==========================================
+ Coverage   84.76%   86.13%   +1.37%     
==========================================
  Files          40       40              
  Lines        8603     9239     +636     
==========================================
+ Hits         7292     7958     +666     
+ Misses       1311     1281      -30     
Files Changed Coverage Δ
src/widgets/paragraph.rs 99.81% <100.00%> (ø)

... and 9 files with indirect coverage changes

@orhun
Copy link
Sponsor Member

orhun commented Jul 30, 2023

Type aliases are not shown in rustdocs so I'm not sure if it would be useful to add them. I think only adding a comment to scroll method might be sufficient.

Let's also hear what other maintainers think.

src/widgets/paragraph.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Member

joshka commented Aug 4, 2023

Two different renderings:

When there are values already specified:
image

When there isn't yet a value:
image

So I guess that there's some value to this, even if it doesn't end up in the docs.

@joshka
Copy link
Member

joshka commented Aug 18, 2023

I think based on some discussions in discord and github we should add a bit more docs to the doc comment around this rather than just a single line comment. It should cover:

  • The scroll position renders text by ...
  • Scroll position's interaction with wrapping ...
  • How bounds checks work (e.g. scrolling beyond the number of lines of text)
  • Links to scroll RFC - to help push for feedback
  • It doesn't have any scrollbar (see the scrollbar widget)
  • The elements are out of order (vertical/horizontal) compared the rest of ratatui (h/v)

@hasezoey
Copy link
Contributor Author

updated to have:

  • Links to scroll RFC - to help push for feedback
  • The elements are out of order (vertical/horizontal) compared the rest of ratatui (h/v)
  • The scroll position renders text by ... (partially?)

as for the other things, i dont know enough about it to write text for it (like how wrapping is decided, interacts with scroll or how scroll is clamped)

It doesn't have any scrollbar (see the scrollbar widget)

should this really be mentioned?

@joshka
Copy link
Member

joshka commented Aug 19, 2023

LGTM - no problem with the other stuff.

@kdheepak kdheepak added this pull request to the merge queue Aug 19, 2023
Merged via the queue into ratatui-org:main with commit ab5e616 Aug 19, 2023
30 checks passed
@hasezoey hasezoey deleted the docScrollFields branch August 20, 2023 10:14
@joshka joshka added this to the v0.23.0 milestone Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants