Feat: Share QR Code #18636
Feat: Share QR Code #18636
Conversation
|
@robertjchen Updated the screenshots in "iOS" and "Web" |
|
@robertjchen looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
All checks were passing at time of merge. No cherry-pick label was present, so that check is not applicable. 
|
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 1.3.14-1 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
| style={{ | ||
| alignSelf: 'stretch', | ||
| height: 27, | ||
| marginBottom: 20, | ||
| }} |
There was a problem hiding this comment.
Hi everyone, coming from a follow-up PR for this component. Just wanted to note that as per our styling guidelines, let's remember to avoid inline styles. Thanks!
There was a problem hiding this comment.
got it, thanks for the feedback! 👍
| render() { | ||
| const isReport = this.props.report != null && this.props.report.reportID != null; | ||
|
|
||
| const url = isReport ? `${CONST.NEW_EXPENSIFY_URL}r/${this.props.report.reportID}` : `${CONST.NEW_EXPENSIFY_URL}details?login=${this.props.session.email}`; |
There was a problem hiding this comment.
There was a problem hiding this comment.
We used a fixed CONST.NEW_EXPENSIFY_URL here, which caused an issue as it didn't set the URL based on the environment.
| <Text | ||
| family="EXP_NEW_KANSAS_MEDIUM" | ||
| fontSize={22} | ||
| style={{marginTop: 15}} | ||
| > | ||
| {this.props.title} | ||
| </Text> |
There was a problem hiding this comment.
This PR caused a regression here
|
We expect menu item along with the icon to turn white when hovered. But we missed to implement it for Share Code. Issue: #19067 |
| url={url} | ||
| title={isReport ? this.props.report.reportName : this.props.currentUserPersonalDetails.displayName} | ||
| subtitle={isReport ? ReportUtils.getPolicyName(this.props.report) : this.props.session.email} | ||
| logo={isReport ? roomAvatar : this.props.currentUserPersonalDetails.avatar} |
There was a problem hiding this comment.
This line caused a regression in #19211. When we delete the avatar, the optimistic avatar is set to the oldDot default avatar. After a second, the avatar is updated to the newDot default avatar. We fixed this issue by using a utility function that always returns the newDot default avatar.
| {this.props.title} | ||
| </Text> | ||
|
|
||
| {this.props.subtitle && ( |
There was a problem hiding this comment.
@chrispader @robertjchen Because we didn't use boolean for conditional render, it caused a regression (although dev), here. It is essential for conditional render to use boolean expressions to avoid console errors.
I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && .
We also have an item in the checklist which was marked but we missed highlighting this one.
There was a problem hiding this comment.
Got it, thanks for the feedback! 🙇
|
While adding this feature of sharing QR code, we missed to handle the case where an user might not have access to the page. This issue was reported #21157. Issue: Web - random QR code is generated on opening share code of room using URL |
| const url = isReport ? `${CONST.NEW_EXPENSIFY_URL}r/${this.props.report.reportID}` : `${CONST.NEW_EXPENSIFY_URL}details?login=${this.props.session.email}`; | ||
|
|
||
| const platform = getPlatform(); | ||
| const isNative = platform === CONST.PLATFORM.IOS || platform === CONST.PLATFORM.ANDROID; |
There was a problem hiding this comment.
@chrispader This is against our guide on how to do platform code. Please refer to https://github.com/Expensify/App#platform-specific-file-extensions and submit a PR that refactors this appropriately.
There was a problem hiding this comment.
This is actually just a temporary solution until QR code downloads are also available on web. We're currently working on that here.
Once this is possible, the platform-specific code will be removed.
There was a problem hiding this comment.
Yep, we unfortunately had to push this out in time for EC3- but, we're cleaning it up as soon as possible once we figure out the CORs issue.
Details
Fixed Issues
$ #18630
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
QR Code in Personal Settings
Sharing QR Code from Personal Settings
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting 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)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android