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

fix(layout): cap Contstraint::apply to 100% length #264

Merged
merged 1 commit into from
Jun 17, 2023

Conversation

endepointe
Copy link
Contributor

@endepointe endepointe commented Jun 15, 2023

This function is only currently used in by the chart widget for constraining the width and height of the legend.

@endepointe endepointe requested a review from joshka as a code owner June 15, 2023 18:11
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #264 (82ac8f8) into main (6c2fbbf) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
+ Coverage   81.76%   81.92%   +0.15%     
==========================================
  Files          34       34              
  Lines        6591     6627      +36     
==========================================
+ Hits         5389     5429      +40     
+ Misses       1202     1198       -4     
Impacted Files Coverage Δ
src/layout.rs 87.73% <100.00%> (+2.48%) ⬆️

@endepointe endepointe marked this pull request as draft June 15, 2023 18:55
@joshka
Copy link
Member

joshka commented Jun 15, 2023

Thanks for your submission.
A couple of quick suggestions:

  • use clamp() instead of chaining min and max
  • remove the comments including the todo comment
  • please add a unit test in the same file to verify the behavior of the change.

When you update the PR, to keep it ready to merge please ensure that you squash your commits and force push them onto your remote branch. This helps keeps the main branch uncluttered.

@endepointe
Copy link
Contributor Author

premature fix. my mistake.

@endepointe endepointe closed this Jun 15, 2023
@endepointe endepointe reopened this Jun 15, 2023
This function is only currently used in by the chart widget for
constraining the width and height of the legend.
@joshka joshka marked this pull request as ready for review June 17, 2023 03:06
@joshka joshka changed the title add range restriction to src/layout.rs:44 fix(layout): cap Contstraint::apply to 100% length Jun 17, 2023
@joshka
Copy link
Member

joshka commented Jun 17, 2023

Hey, I made some updates for this (and updated the PR title / message). Thanks for calling out the bug.

@joshka
Copy link
Member

joshka commented Jun 17, 2023

Lint issue is fixed in #266

@joshka joshka added this pull request to the merge queue Jun 17, 2023
Merged via the queue into ratatui:main with commit 28a8435 Jun 17, 2023
samyosm pushed a commit to samyosm/ratatui that referenced this pull request Jun 18, 2023
This function is only currently used in by the chart widget for
constraining the width and height of the legend.
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.

3 participants