Feat: Added Tooltip for Jump to msg button#751
Feat: Added Tooltip for Jump to msg button#751dhairyashiil wants to merge 8 commits intoRocketChat:developfrom
Conversation
|
"Send Message" -> A tooltip for send icon is unnecessary, pls remove that. For the Go to Message, change it to Jump to Message, Let's be consistent with Rocket.Chat channel terminologies |
Spiral-Memory
changed the title
|
Hello @Spiral-Memory, |
| font-family: sans-serif; | ||
| top: ${position === 'top' ? 'calc(-100% - 20px)' : 'calc(100% + 10px)'}; | ||
|
|
||
| top: ${position === 'top' |
There was a problem hiding this comment.
Any specific reason we need this huge change for this ?
I am just worried that it should not affect the other existing tooltips
There was a problem hiding this comment.
Yes, I double-checked the changed code. I also tried reducing the code, but I was unable to reduce many lines because, earlier, we were only considering the top and bottom positions, and we were hardcoding the CSS values for the left and transform properties in the tooltip. Now, we have 4 positions and are setting the required fields dynamically.
There was a problem hiding this comment.
Okay,
That is fine, but the code is not very readable
Any other way we can have this dynamic positioning
There was a problem hiding this comment.
Okay, Let's see. I will look into it.
|
Hello @Spiral-Memory, Please refer to the video in which I have shown that all the tooltips are working as expected. all.tooltips.mp4and
|
| onClick={() => setJumpToMessage(msg._id)} | ||
| css={{ | ||
| position: 'relative', | ||
| zIndex: 10, |
There was a problem hiding this comment.
Any specific reason to hardcode this zIndex ?
The thing is, if you notice, we are using specific set of zIndex mentioned in the documentation
We might want to align with that standard
There was a problem hiding this comment.
No, that z-index logic was not added by me. I added only the tooltip part here.
There was a problem hiding this comment.
Okay cool then, still not sure how it is there.. there was specific set of z-indexes I made after my gsoc..
That's fine
Will see..
There was a problem hiding this comment.
I see PR #667. It looks like this hardcoded code came from there. No worries, I’ll fix the logic in the current PR.
There was a problem hiding this comment.
@Spiral-Memory, what value should I set for zIndex?
I can see that in the codebase, the zIndex value can come from the theme, but if not, we’re maintaining specific zIndex values for certain components, like:
z-index: ${theme.zIndex?.sidebar || 1200};
z-index: ${theme.zIndex?.menu || 1300};
z-index: ${theme.zIndex?.tooltip || 1400};
z-index: ${theme.zIndex?.modal || 1500};
z-index: ${theme.zIndex?.toastbar || 1600};
| font-family: sans-serif; | ||
| top: ${position === 'top' ? 'calc(-100% - 20px)' : 'calc(100% + 10px)'}; | ||
|
|
||
| top: ${position === 'top' |
There was a problem hiding this comment.
Okay,
That is fine, but the code is not very readable
Any other way we can have this dynamic positioning
packages/react/src/views/MessageAggregators/common/MessageAggregator.js
Outdated
Show resolved
Hide resolved
Spiral-Memory
force-pushed
the
develop
branch
from
2ef2985 to
8f3bb40
Compare
2 participants
Acceptance Criteria fulfillment
Fixes #750
Screenshots
Send Button:
Starred Messages:
Pinned Messages:
PR Test Details
Note: The PR will be ready for live testing at https://rocketchat.github.io/EmbeddedChat/pulls/pr-751 after approval. Contributors are requested to replace
<pr_number>with the actual PR number.