feat(popup): restyle popups - #5351
Conversation
dastarruer
changed the title
|
More screenshots! Dark mode
Light mode(Just realized I need to style the highlight type buttons, but here's a screenshot nonetheless): 
|
|
How do I update the tests so checks pass? (Pre-push hook passed on my machine so I didn't realize my mistake) |
|
Remove the PNG files that serve as the reference images for the highlight rendering tests. Running pnpm test:browser on your local machine will regenerate them. Once you commit the regenerated images, the CI will use them as the new baseline for rendering comparisons. |
Yes, it should be restyled with theme colors too. |
Also realized that these toggles look reversed, so I'll need to fix that as well before this can be merged. |
| const innerTriangleStyles = getTriangleStyles(trianglePosition, 7, -1); | ||
|
|
||
| const popupHeight = height ?? childrenHeight; | ||
| const triangleHidden = !!( |
There was a problem hiding this comment.
If this is removed, triangle might be drawn at a place where no text is selected, especially when the popup is higher than half of the screen and the selected text is at the center of the screen, it could be reproduced with the dictionary popup on smaller screens.
There was a problem hiding this comment.
2026-07-27.10-50-02.mp4
Can't seem to replicate it. Is this what you meant?
There was a problem hiding this comment.
This can be reproduced with a wider but lower window so that the popup is a modal instead of an action sheet.
There was a problem hiding this comment.
I can't reproduce it at all:
If you can, please send a screenshot so I know what you're talking about.
There was a problem hiding this comment.
Note that for the 'floated' word, the triangle is hidden otherwise it will be drawn at a wrong place.
There was a problem hiding this comment.
Still can't reproduce it. But the reason I removed it was to fix a visual bug:
I'll see if I can't work around it, but I don't really know why this is happening.
There was a problem hiding this comment.
The triangle color might be hardcoded to gray-500.
There was a problem hiding this comment.
Doesn't seem to be the case anywhere. The effect I'm trying to get (which you can see in the screenshots) only seems to work once both inner and outer triangles are set to z-50.
Again, can't seem to reproduce the bug. Maybe instead of hiding the triangle all popups could get a fixed height, so the triangle never needs to be hidden? Either way will keep working at it.
There was a problem hiding this comment.
Again, can't seem to reproduce the bug. Maybe instead of hiding the triangle all popups could get a fixed height, so the triangle never needs to be hidden? Either way will keep working at it.
There are still cases where the popup is more than half the height of the window. When the selected text is near the center of the window, the popup’s pointer triangle can therefore be misplaced. It will be reproducible with a higher popup.
I took the liberty of redoing the buttons instead: 
Mostly taken from gdocs: 
|
7db5f38 to
afd202e
Compare
Please keep the original highlight style, it was much better than this. 
This may be an existing issue, but the shape should be a perfect circle. |
The original style had poor contrast: 
Since the user can choose literally any color, choosing a text color that properly contrasts is difficult. Hence, I took the approach that gdocs does, and just made it a colored underline.
Yes, I’ll fix this. It was an issue introduced by this pr. |
2 participants
Closes #5267.
Screenshots
Dark mode
Light mode