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(block): fix crash on empty right aligned title #933

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Conversation

joshka
Copy link
Member

@joshka joshka commented Feb 7, 2024

  • Simplified implementation of the rendering for block.
  • Introduces a subtle rendering change where centered titles that are
    odd in length will now be rendered one character to the left compared
    to before. This aligns with other places that we render centered text
    and is a more consistent behavior. See
    feat(text): add style and alignment #807 (comment)
    for another example of this.

Fixes: 929

fix: tests

- Simplified implementation of the rendering for block.
- Introduces a subtle rendering change where centered titles that are
  odd in length will now be rendered one character to the left compared
  to before. This aligns with other places that we render centered text
  and is a more consistent behavior. See
  #807 (comment)
  for another example of this.

Fixes: 929

fix: tests
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (0dcdbea) 92.0% compared to head (94bea39) 92.0%.
Report is 1 commits behind head on main.

Files Patch % Lines
src/widgets/block.rs 94.1% 9 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #933     +/-   ##
=======================================
- Coverage   92.0%   92.0%   -0.1%     
=======================================
  Files         61      61             
  Lines      15582   15621     +39     
=======================================
+ Hits       14345   14375     +30     
- Misses      1237    1246      +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member Author

@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.

Marked the 3 meaningful changes in the centering behavior.
All other test changes were to make the tests more readable and were checked before and after as being correct.

tests/widgets_block.rs Show resolved Hide resolved
tests/widgets_block.rs Show resolved Hide resolved
tests/widgets_block.rs Show resolved Hide resolved
@kdheepak
Copy link
Collaborator

kdheepak commented Feb 7, 2024

LGTM! Let's mark it as a breaking change though.

src/widgets/block.rs Outdated Show resolved Hide resolved
Co-authored-by: Dheepak Krishnamurthy <me@kdheepak.com>
Copy link
Member

@Valentin271 Valentin271 left a comment

Choose a reason for hiding this comment

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

I like the render refactor in this, way easier to read.

I wouldn't mark this as breaking change, not feeling strongly about it though, up to you.

@kdheepak
Copy link
Collaborator

kdheepak commented Feb 7, 2024

If a user has tests for title, their tests will break with no changes to their code. I think it'd be nice to let people know that the behavior has changed, for the better.

@Valentin271
Copy link
Member

If we decide it's breaking we should wait after the point release then.

@kdheepak
Copy link
Collaborator

kdheepak commented Feb 7, 2024

Hmm. Maybe it is not worth waiting that long? Sorry for going back on this, I didn't consider the release timeline. I think if we make it prominent in the changelog, I'm fine with calling it "not a breaking change" but a "bug fix" instead, and getting it in in this release.

@Valentin271
Copy link
Member

I don't feel strongly either way. This is technically breaking, but the old behavior was wrong so ... This so small no one will notice it (except unit tests). Might be better to include the fix as soon as possible to correct wrong behavior.

@joshka
Copy link
Member Author

joshka commented Feb 7, 2024

I don't think rendering changes should be marked as breaking either from a perspective of recording in the the breaking changes doc or causing a major release. Every change we make changes the way that things render in some way.

@joshka joshka merged commit 2202059 into main Feb 7, 2024
35 of 37 checks passed
@joshka joshka deleted the jm/fix-block branch February 7, 2024 23:24
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.

Crash when rendering a block with an empty title with alignment right
3 participants