-
Notifications
You must be signed in to change notification settings - Fork 214
Avoid annotation buttons from overflowing the card #7337
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7337 +/- ##
==========================================
+ Coverage 99.47% 99.51% +0.04%
==========================================
Files 271 271
Lines 10968 10977 +9
Branches 2620 2621 +1
==========================================
+ Hits 10910 10924 +14
+ Misses 58 53 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
78055f3 to
a0d465f
Compare
robertknight
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 . I suspect we might have a use for the hook in other projects in future.
| 'rounded-r-none', | ||
| // Truncate text in this button. It also requires overwriting its | ||
| // `display` property from flex to block | ||
| 'truncate !block', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify why the display change is needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Adding text-overflow: ellipsis to a display: flex element without a "known" width does not work.
Grabacion.de.pantalla.desde.2025-09-30.09-59-21.mp4
A way to avoid having to override defaults from the Button component, would be to add a sub-element with truncate, but then we have to set explicit max and min widths to the button.
diff --git a/src/sidebar/components/Annotation/AnnotationPublishControl.tsx b/src/sidebar/components/Annotation/AnnotationPublishControl.tsx
index bcca6e570..59f7fb823 100644
--- a/src/sidebar/components/Annotation/AnnotationPublishControl.tsx
+++ b/src/sidebar/components/Annotation/AnnotationPublishControl.tsx
@@ -84,9 +84,7 @@ function AnnotationPublishControl({
classes={classnames(
// Turn off right-side border radius to align with menu-open button
'rounded-r-none',
- // Truncate text in this button. It also requires overwriting its
- // `display` property from flex to block
- 'truncate !block',
+ 'max-w-full min-w-0',
)}
data-testid="publish-control-button"
style={buttonStyle}
@@ -97,7 +95,7 @@ function AnnotationPublishControl({
title={isButtonContentTruncated ? postButtonText : undefined}
elementRef={postButtonRef}
>
- {postButtonText}
+ <div className="truncate">{postButtonText}</div>
</Button>
{/* This wrapper div is necessary because of peculiarities with
Safari: see https://github.com/hypothesis/client/issues/2302 */}The "problem" is that we already had to set min and max widths to the parent element, and it becomes a bit cumbersome and unintuitive when you have to set it at various levels.
a0d465f to
8c55562
Compare
8c55562 to
cd85bd8
Compare
Fixes #7250
Truncate annotation post button content to ensure the buttons do not overflow the thread card.
Additionally, dynamically add a title to the button if the content is in fact being truncated.
Grabacion.de.pantalla.desde.2025-09-25.10-21-28.mp4
This is how it looks in main branch without these changes:
Considerations
I added a side effect with a resize observer that checks if the content of the button is being in fact truncated, and in that case adding a title to it. However, this might be overkill, maybe it's ok to just add the title unconditionally.We decided to go ahead with this conditional behavior.TODO