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(Block): add documentation to Block #469

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

Valentin271
Copy link
Member

Add documentation to the Block widget and its sub-components (Padding, Borders).

Relates to #386

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #469 (5856bdf) into main (572df75) will increase coverage by 0.06%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #469      +/-   ##
==========================================
+ Coverage   90.00%   90.06%   +0.06%     
==========================================
  Files          40       40              
  Lines       11156    11166      +10     
==========================================
+ Hits        10041    10057      +16     
+ Misses       1115     1109       -6     
Files Changed Coverage Δ
src/title.rs 100.00% <ø> (ø)
src/widgets/block.rs 93.44% <ø> (ø)

... and 3 files with indirect coverage changes

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I've marked the PR as approved, and we can merge it as-is, but I'd also be happy to keep this open to take whatever changes you see as useful based on the feedback below.

I have lots of little comments on this, but none of them are super important / blocking, so please read them as discussion points.

src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Show resolved Hide resolved
src/widgets/block.rs Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
src/widgets/block.rs Outdated Show resolved Hide resolved
add documentation to the `Block` widget and its sub-components
@Valentin271
Copy link
Member Author

I fixed most of your comments. I just left out the comments about the style and style prioritization. I agree that we should use a consistent language and document the style priorities. But I think this would be better documented in the style module (mainly the way we merge style), then any method could reference that as a base and add on top of it.

As for the consistent language, we could add to the guideline, for example:

  • Setters that replace values use this sentence: 'Sets xxx'
  • Setters that add default values use this sentence: 'Sets the default for xxx'

Put in other words, some sort of template.
Let me know what you think. I think consistency is very important, but how do we enforce it across multiple collaborators.

@joshka
Copy link
Member

joshka commented Sep 6, 2023

Thanks - I'll slap together a PR that fixes up the remaining wording for styles.

@joshka joshka merged commit 0c68ebe into ratatui:main Sep 6, 2023
31 checks passed
@joshka
Copy link
Member

joshka commented Sep 6, 2023

I removed the commit message body, as I don't think it really added much info to the changelog that wouldn't be answered by reading the commit title.

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.

4 participants