This repository was archived by the owner on Mar 7, 2026. It is now read-only.
Always show highlight buttons#397
Merged
joseplayero merged 1 commit intoreorproject:mainfrom Sep 5, 2024
Merged
Conversation
There was a problem hiding this comment.
PR Summary
This pull request addresses the issue of the highlight utility button not displaying when highlighted text overflows the screen height, modifying the button's positioning logic in the HighlightExtension component.
- Modified
src/components/Editor/HighlightExtension.tsxto adjust button positioning when text overflows screen height - Introduced a conditional check
top >= window.innerHeightto determine button placement - Set button position to 20px from top of screen when text overflows, otherwise 35px above highlighted text
- Removed a console.log statement, improving code cleanliness
- Potential UI/UX implications of new button positioning warrant further discussion and testing
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
|
|
||
| // Calculate the button position below the last word | ||
| const buttonTop = top - 35 // Adjust the vertical offset as needed | ||
| const buttonTop = top >= window.innerHeight ? 20 : top - 35 // Adjust the vertical offset as needed |
There was a problem hiding this comment.
style: Consider using a constant for the 20px value for better maintainability
|
|
||
| // Calculate the button position below the last word | ||
| const buttonTop = top - 35 // Adjust the vertical offset as needed | ||
| const buttonTop = top >= window.innerHeight ? 20 : top - 35 // Adjust the vertical offset as needed |
There was a problem hiding this comment.
logic: The button might still be off-screen if the window is very short. Consider adding a minimum value check
| // Calculate the button position below the last word | ||
| const buttonTop = top - 35 // Adjust the vertical offset as needed | ||
| const buttonTop = top >= window.innerHeight ? 20 : top - 35 // Adjust the vertical offset as needed | ||
| const buttonLeft = (left + right) / 2 - 190 // Position the button horizontally centered |
There was a problem hiding this comment.
style: The -190 offset seems arbitrary. Consider calculating this dynamically based on button width
joseplayero
approved these changes
Sep 5, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #348
/claim #348
I think it might be a product decision to show the highlight utility button when texts overflows, personally I think it would be better to display at the top but open for discussion
Screen.Recording.2024-09-04.at.10.09.42.PM.mov