Add feature to view Profile Picture in full screen #8483
Conversation
melvin-bot
bot
requested review from
Julesssss and
parasharrajat
and removed request for
a team
src/components/AttachmentModal.js
Outdated
| const defaultProps = { | ||
| isUploadingAttachment: false, | ||
| isProfilePicture: false, | ||
| sourceURL: null, | ||
| onConfirm: null, | ||
| originalFileName: null, |
There was a problem hiding this comment.
I wonder if AttachmentModal still makes sense as a name for this component...
|
I agree. At this point, upload functionality should be separated from the preview component and we should change the name of this component. |
Julesssss
dismissed
their stale review
Checked with Design, the square default images is okay for now
|
Okay, so ignore my change request about the square default images -- we'll deal with that some other time. |
|
@Julesssss @parasharrajat Any Suggestion for the name? @parasharrajat Are you suggesting to make two separate components one for 'uploading attachment' and other for 'previewing attachment and profile picture'? I don't see the need. |
This is good to have for separation of concern. You can use composition. |
|
Opened the discussion here https://expensify.slack.com/archives/C01GTK53T8Q/p1649263301271969. You can share your opinion there. |
|
Ok, others seem to agree with my change request here. More on slack. |
|
Hello @parasharrajat, I will be away from my laptop for next 3-4 days. So, will push the update after 3-4 days. |
|
Thank You for Your Patience. So, here we will be adding two new components: import _ from 'underscore';
import React from 'react';
import PropTypes from 'prop-types';
import AttachmentModal from './AttachmentModal';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
const propTypes = {
/** Determines title of the modal header depending on if we are viewing a profile picture or not */
isProfilePicture: PropTypes.bool,
/** DisplayName to be used as headerTitle if isProfilePicture is true. */
displayName: PropTypes.string,
...withLocalizePropTypes,
};
const defaultProps = {
isProfilePicture: false,
displayName: null,
};
const PreviewAttachmentModal = (props) => {
const propsToPass = _.omit(props, 'displayName', 'isProfilePicture');
return (
<AttachmentModal
headerTitle={props.isProfilePicture ? props.displayName : props.translate('common.attachment')}
shouldShowDownloadButton={!props.isProfilePicture}
/* eslint-disable-next-line react/jsx-props-no-spreading */
{...propsToPass}
/>
);
};
PreviewAttachmentModal.propTypes = propTypes;
PreviewAttachmentModal.defaultProps = defaultProps;
PreviewAttachmentModal.displayName = 'PreviewAttachmentModal';
export default withLocalize(PreviewAttachmentModal);and /src/components/UploadAttachmentModal.js import React from 'react';
import AttachmentModal from './AttachmentModal';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
const propTypes = {
...withLocalizePropTypes,
};
const UploadAttachmentModal = props => (
<AttachmentModal
headerTitle={props.translate('reportActionCompose.sendAttachment')}
/* eslint-disable-next-line react/jsx-props-no-spreading */
{...props}
/>
);
UploadAttachmentModal.propTypes = propTypes;
UploadAttachmentModal.displayName = 'UploadAttachmentModal';
export default withLocalize(UploadAttachmentModal);Do we need any other changes? |
|
Your suggestion is fine but does not add much value. I think it would be better to just create a simple component called AttachmentPreviewModal from the AttachmentModal without all the complex upload logic and use OR see the requested changes. |
src/libs/actions/PersonalDetails.js
Outdated
| @@ -100,6 +100,7 @@ function formatPersonalDetails(personalDetailsList) { | |||
| const lastName = details.lastName || ''; | |||
| const payPalMeAddress = details.expensify_payPalMeAddress || ''; | |||
| const phoneNumber = details.phoneNumber || ''; | |||
| const avatarHighResolution = details.avatar || OptionsListUtils.getDefaultAvatar(login); | |||
There was a problem hiding this comment.
Why do we need this new property? And How come the default avatar is used here? we should depend on details.avatar. If a user has a default avatar, shouldn't it be available in details.avatar
There was a problem hiding this comment.
const avatar = getAvatar(details, sanitizedLogin);
function getAvatar(personalDetail, login) {
if (personalDetail && personalDetail.avatarThumbnail) {
return personalDetail.avatarThumbnail;
}
return OptionsListUtils.getDefaultAvatar(login);
}
avatar returned from formatPersonalDetails function uses avatarThumbnail and is of lower resolution for user uploaded images. So, new property is needed.
const avatarHighResolution = details.avatar || details.avatarThumbnail;
As details.avatar is undefined incase of default avatar. But, details.avatarThumbnail has the needed url. So, we can use details.avatar || details.avatarThumbnail;
| source={details.avatar} | ||
| /> | ||
| <AttachmentModal | ||
| sourceURL={details.avatarHighResolution} |
There was a problem hiding this comment.
Can't we do that directly here?
There was a problem hiding this comment.
details.avatar being used here is avatar returned from formatPersonalDetails function which is of lower resolution. So, new property for higher resolution is added in formatPersonalDetails function which is later used here.
| @@ -109,6 +110,7 @@ function formatPersonalDetails(personalDetailsList) { | |||
| timezone, | |||
| payPalMeAddress, | |||
| phoneNumber, | |||
| avatarHighResolution, | |||
There was a problem hiding this comment.
I think it would be better to use the avatarThumbnail and avatar properties. which means use avatarThumbnail instead of avatar and store details.avatar in the avatar.
Now use avatarThumbnail where the avatar was used.
There was a problem hiding this comment.
Now use avatarThumbnail where the avatar was used.
@parasharrajat Do we want to do this change here? This might require massive code refactoring.
There was a problem hiding this comment.
I was expecting it to be done here because of the keys. But I don't mind another PR doing that. At last, I am fine with this as well if @Julesssss is.
There was a problem hiding this comment.
Yeah, I think that's okay.
| source={details.avatar} | ||
| /> | ||
| <PreviewAttachmentModal | ||
| sourceURL={details.avatarHighResolution} |
There was a problem hiding this comment.
Instead of this directly use sourceURL={details.avatar || details.avatarThumbnail} here
src/components/AttachmentModal.js
Outdated
| title={this.props.isUploadingAttachment | ||
| ? this.props.translate('reportActionCompose.sendAttachment') | ||
| : this.props.translate('common.attachment')} | ||
| title={this.props.headerTitle} |
There was a problem hiding this comment.
this should be like this.
| title={this.props.headerTitle} | |
| title={this.props.headerTitle || this.props.translate('common.attachment')} |
| <AttachmentModal | ||
| headerTitle={props.isProfilePicture ? props.displayName : props.translate('common.attachment')} | ||
| allowDownload={!props.isProfilePicture} | ||
| /* eslint-disable-next-line react/jsx-props-no-spreading */ | ||
| {...propsToPass} | ||
| /> |
There was a problem hiding this comment.
Lets remove this component and directly use AttachmentModal and set the headerTitle & allowDownload accordingly.
| ...withLocalizePropTypes, | ||
| }; | ||
|
|
||
| const UploadAttachmentModal = props => ( |
There was a problem hiding this comment.
Lets remove this component and directly use AttachmentModal and set the headerTitle & allowDownload accordingly.
These components are not adding much value.
|
I saw that you marked Comments resolved but I don't see any updates on those. |
|
Please improve the QA steps. |
yeah. 😅 |
|
So the only part of the checklist that didn't pass was the offline part. But this matches the existing behavior for Profile pics/ Attachments so it's not a problem in my opinion. Merging! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by @sketchydroide in version: 1.1.57-8 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by @chiragsalian in version: 1.1.57-17 🚀
|
|
This has caused a regression in #17454 (comment) (#16528) |
|
@eVoloshchak I can't find anything related to what you explained above in the changes. Can you point it out? |
|
@parasharrajat, sure! 
|
Details
New Feature: Added the ability to view profile pictures in full screen/larger size by clicking/tapping on them
Fixed Issues
$ #8197
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly 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
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly 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)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android