-
Notifications
You must be signed in to change notification settings - Fork 3k
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
7486 reduce tooltip over group #8494
7486 reduce tooltip over group #8494
Conversation
Signed-off-by: LucioChavezFuentes <luciobertinchavez@gmail.com>
Signed-off-by: LucioChavezFuentes <luciobertinchavez@gmail.com>
3bc19a5
to
268d035
Compare
268d035
to
6e0bfe6
Compare
Also, this tooltip settings needs to be applied a few more locations.
|
Apply Tooltip settings on DisplayNames.js, it seems it cover all. I checked the app fairly well and I didn't find more tooltps that need this setting but if you find one let me know. |
I moved the max-width condition and explained why we can't use onLayout of the Text or measure 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.
Testting..
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.
Thanks @parasharrajat, I updated the comment and change |
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.
Just a few cosmetic / comment changes requested, overall looks fantastic & tests well! 👍
PR updated and ready to be reviewed. |
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.
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.
Thanks for sticking with us through all these requested changes 👍 Getting super close!
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.
Let's get this merged, thanks for sticking with this!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Thank you @parasharrajat and @Beamanator for sticking with me. I know it's been a long thread and I really appreciate your time on many reviews on this pull request. |
🚀 Deployed to staging by @Beamanator in version: 1.1.58-0 🚀
|
🚀 Deployed to production by @AndrewGable in version: 1.1.60-3 🚀
|
Details
My changes consists of:
maxWidth
andnumberOfLines
props.componentDidMount
.measureTooltip
function,tooltipWidth
andtooltipHeight
states fromTooltip/index.js
toTooltipRenderedOnPageBody
component.Fixed Issues
$ #7486
Tests
Add enough users to the group so the amount of emails' texts could make a large text.
Hover over MultipleAvatars icon of your recently created group on Chats side bar.
variables.sideBarWidth
should appear with no extra white space on right side of emails.a. Chat title end elipsis (...).
b. On Report Header title end elipsis

c. chat title end elipsis on Search Page.

PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
The steps are the same of Tests section.
Screenshots
Web
Mobile Web
N/A
Desktop
iOS
N/A
Android
N/A