fix URL address popup won’t show when hovered on terms/privacy links on about page #12434
Conversation
melvin-bot
bot
requested review from
Beamanator and
eVoloshchak
and removed request for
a team
|
@eVoloshchak I know you've been pretty busy recently, do you have time to review this or should I go ahead? |
I have the time, going to start reviewing this soon, thanks! |
| style={[styles.textMicroSupporting, styles.link]} | ||
| onPress={() => Link.openExternalLink(CONST.TERMS_URL)} | ||
| > | ||
| <TextLink style={[styles.textMicroSupporting, styles.link]} href={CONST.TERMS_URL}> |
There was a problem hiding this comment.
It fells like we should remove styles.link since it's already included in TextLink. However, doing so would break the styling. On this line
App/src/components/TextLink.js
Line 61 in e434efa
blue color from
styles.link is overriden with grey color from additionalStyles (styles.textMicroSupporting)We could just swap the order like this
style={[...additionalStyles, styles.link]}but I'm afraid it will break all of the other usages of TextLink throughout the app.
@0xmiroslav, do you some ideas on how this can be resolved without introducing lots of changes?
There was a problem hiding this comment.
If we want to remove styles.link but also not change lots of codes, maybe add new style like this:
textLink: {
fontSize: variables.fontSizeSmall,
lineHeight: 14,
},
so that style looks like:
<TextLink style={styles.textLink}
But I think no problem with [styles.textMicroSupporting, styles.link]
There's already similar style definitions in other parts.
i.e. https://github.com/Expensify/App/blob/main/src/pages/signin/TermsAndLicenses.js
There was a problem hiding this comment.
There's already similar style definitions in other parts.
True
Let's keep the styles then
|
|
@Beamanator looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
|
Saw screenshots & approved checklists so I didn't notice tests weren't all passing 🙃 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @Beamanator in version: 1.2.25-0 🚀
|
|
🚀 Deployed to production by @yuwenmemon in version: 1.2.25-0 🚀
|
Details
replace Text with TextLink which is already used in other pages
Fixed Issues
$ #12238
PROPOSAL: #12238 (comment)
Tests
QA Steps
PR Review Checklist
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)Screenshots
Web
web.mov
Mobile Web - Chrome
mchrome.mp4
Mobile Web - Safari
msafari.mp4
Desktop
desktop.mov
iOS
ios.mp4
Android
android.mp4