Fix: Popover reaction list does not update dynamically #24024
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
…popover-reaction-list-not-update-dynamically
…popover-reaction-list-not-update-dynamically
s77rt
suggested changes
Aug 2, 2023
src/pages/home/report/ReactionList/PopoverReactionList/index.js
Outdated
Show resolved
Hide resolved
src/pages/home/report/ReactionList/PopoverReactionList/index.js
Outdated
Show resolved
Hide resolved
s77rt
suggested changes
Aug 3, 2023
src/pages/home/report/ReactionList/PopoverReactionList/index.js
Outdated
Show resolved
Hide resolved
6 tasks
…popover-reaction-list-not-update-dynamically
|
@s77rt Please check the |
|
@tienifr Thanks! Overall looks good, I just have some optimisation suggestions:
I have included a diff where most of those changes are implemented. There is also a console.log at render to track the performance. Verify that every click / action results in exactly one render only (with the exception of hiding the popover when last user is removed). Code diffdiff --git a/src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.js b/src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.js
index 2f6f112d55..67ffb622ea 100644
--- a/src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.js
+++ b/src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.js
@@ -1,5 +1,6 @@
import React from 'react';
import {Dimensions} from 'react-native';
+import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import _ from 'underscore';
import {withOnyx} from 'react-native-onyx';
@@ -16,12 +17,17 @@ import ONYXKEYS from '../../../../../ONYXKEYS';
import EmojiReactionsPropTypes from '../../../../../components/Reactions/EmojiReactionsPropTypes';
const propTypes = {
+ reportActionID: PropTypes.string,
+ emojiName: PropTypes.string,
+
emojiReactions: EmojiReactionsPropTypes,
...withLocalizePropTypes,
};
const defaultProps = {
+ reportActionID: '',
+ emojiName: '',
emojiReactions: {},
};
@@ -41,14 +47,8 @@ class BasePopoverReactionList extends React.Component {
horizontal: 0,
vertical: 0,
},
- users: [],
- emojiCodes: [],
- emojiName: '',
- emojiCount: 0,
- hasUserReacted: false,
};
- this.onPopoverHideActionCallback = () => {};
this.reactionListAnchor = undefined;
this.showReactionList = this.showReactionList.bind(this);
this.hideReactionList = this.hideReactionList.bind(this);
@@ -56,7 +56,6 @@ class BasePopoverReactionList extends React.Component {
this.getReactionListMeasuredLocation = this.getReactionListMeasuredLocation.bind(this);
this.getReactionInformation = this.getReactionInformation.bind(this);
this.dimensionsEventListener = null;
- this.contentRef = React.createRef();
}
componentDidMount() {
@@ -67,8 +66,13 @@ class BasePopoverReactionList extends React.Component {
const previousLocale = lodashGet(this.props, 'preferredLocale', CONST.LOCALES.DEFAULT);
const nextLocale = lodashGet(nextProps, 'preferredLocale', CONST.LOCALES.DEFAULT);
+ if (!this.state.isPopoverVisible && !nextState.isPopoverVisible) {
+ return false;
+ }
+
return (
this.props.reportActionID !== nextProps.reportActionID ||
+ this.props.emojiName !== nextProps.emojiName ||
!_.isEqual(this.props.emojiReactions, nextProps.emojiReactions) ||
!_.isEqual(this.state, nextState) ||
previousLocale !== nextLocale
@@ -76,19 +80,16 @@ class BasePopoverReactionList extends React.Component {
}
componentDidUpdate() {
- // Hide the list when all reactions are removed
- if (this.state.isPopoverVisible && !_.some(lodashGet(this.props.emojiReactions, [this.state.emojiName, 'users']), (user) => !_.isNull(user))) {
- this.hideReactionList();
+ if (!this.state.isPopoverVisible) {
+ return;
}
- const selectedReaction = lodashGet(this.props.emojiReactions, [this.state.emojiName]);
- const {emojiCount, emojiCodes, hasUserReacted, users} = this.getReactionInformation(selectedReaction);
- this.setState({
- users,
- emojiCodes,
- emojiCount,
- hasUserReacted,
- });
+ const isEmptyList = !_.some(lodashGet(this.props.emojiReactions, [this.props.emojiName, 'users']));
+ if (!isEmptyList) {
+ return;
+ }
+
+ this.hideReactionList();
}
componentWillUnmount() {
@@ -123,20 +124,23 @@ class BasePopoverReactionList extends React.Component {
getReactionInformation(selectedReaction) {
if (!selectedReaction) {
return {
- users: [],
- emojiCodes: [],
emojiName: '',
emojiCount: 0,
+ emojiCodes: [],
+ hasUserReacted: false,
+ users: [],
};
}
const reactionUsers = _.pick(selectedReaction.users, _.identity);
const emojiCount = _.map(reactionUsers, (user) => user).length;
const userAccountIDs = _.map(reactionUsers, (user, accountID) => Number(accountID));
- const emoji = EmojiUtils.findEmojiByName(selectedReaction.emojiName);
+ const emojiName = selectedReaction.emojiName;
+ const emoji = EmojiUtils.findEmojiByName(emojiName);
const emojiCodes = EmojiUtils.getUniqueEmojiCodes(emoji, selectedReaction.users);
const hasUserReacted = Report.hasAccountIDEmojiReacted(this.props.currentUserPersonalDetails.accountID, reactionUsers);
const users = PersonalDetailsUtils.getPersonalDetailsByIDs(userAccountIDs, this.props.currentUserPersonalDetails.accountID, true);
return {
+ emojiName,
emojiCount,
emojiCodes,
hasUserReacted,
@@ -149,13 +153,10 @@ class BasePopoverReactionList extends React.Component {
*
* @param {Object} [event] - A press event.
* @param {Element} reactionListAnchor - reactionListAnchor
- * @param {String} emojiName - Name of emoji
*/
- showReactionList(event, reactionListAnchor, emojiName) {
+ showReactionList(event, reactionListAnchor) {
const nativeEvent = event.nativeEvent || {};
this.reactionListAnchor = reactionListAnchor;
- const selectedReaction = lodashGet(this.props.emojiReactions, [emojiName]);
- const {emojiCount, emojiCodes, hasUserReacted, users} = this.getReactionInformation(selectedReaction);
this.getReactionListMeasuredLocation().then(({x, y}) => {
this.setState({
cursorRelativePosition: {
@@ -166,12 +167,7 @@ class BasePopoverReactionList extends React.Component {
horizontal: nativeEvent.pageX,
vertical: nativeEvent.pageY,
},
- users,
- emojiName,
- emojiCodes,
- emojiCount,
isPopoverVisible: true,
- hasUserReacted,
});
});
}
@@ -206,30 +202,30 @@ class BasePopoverReactionList extends React.Component {
}
render() {
+ const selectedReaction = this.state.isPopoverVisible ? lodashGet(this.props.emojiReactions, [this.props.emojiName]) : null;
+ const {emojiName, emojiCount, emojiCodes, hasUserReacted, users} = this.getReactionInformation(selectedReaction);
+ console.log({emojiName});
return (
- <>
- <PopoverWithMeasuredContent
- isVisible={this.state.isPopoverVisible}
+ <PopoverWithMeasuredContent
+ isVisible={this.state.isPopoverVisible}
+ onClose={this.hideReactionList}
+ anchorPosition={this.state.popoverAnchorPosition}
+ animationIn="fadeIn"
+ disableAnimation={false}
+ animationOutTiming={1}
+ shouldSetModalVisibility={false}
+ fullscreen
+ >
+ <BaseReactionList
+ isVisible
+ emojiName={emojiName}
+ users={users}
+ emojiCodes={emojiCodes}
+ emojiCount={emojiCount}
+ hasUserReacted={hasUserReacted}
onClose={this.hideReactionList}
- anchorPosition={this.state.popoverAnchorPosition}
- animationIn="fadeIn"
- disableAnimation={false}
- animationOutTiming={1}
- shouldSetModalVisibility={false}
- fullscreen
- >
- <BaseReactionList
- type={this.state.type}
- isVisible
- users={this.state.users}
- emojiName={this.state.emojiName}
- emojiCodes={this.state.emojiCodes}
- emojiCount={this.state.emojiCount}
- onClose={this.hideReactionList}
- hasUserReacted={this.state.hasUserReacted}
- />
- </PopoverWithMeasuredContent>
- </>
+ />
+ </PopoverWithMeasuredContent>
);
}
}
@@ -243,6 +239,7 @@ export default compose(
withOnyx({
emojiReactions: {
key: ({reportActionID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_REACTIONS}${reportActionID}`,
+ selector: (emojiReactions) => _.pick(emojiReactions, 'heart'),
},
}),
)(BasePopoverReactionList);
diff --git a/src/pages/home/report/ReactionList/PopoverReactionList/index.js b/src/pages/home/report/ReactionList/PopoverReactionList/index.js
index 85c87505a4..608ce1d920 100644
--- a/src/pages/home/report/ReactionList/PopoverReactionList/index.js
+++ b/src/pages/home/report/ReactionList/PopoverReactionList/index.js
@@ -13,6 +13,7 @@ const defaultProps = {
function PopoverReactionList(props) {
const innerReactionListRef = useRef();
const [reactionListReportActionID, setReactionListReportActionID] = useState('');
+ const [reactionListEmojiName, setReactionListEmojiName] = useState('');
/**
* Show the ReactionList modal popover.
@@ -24,7 +25,8 @@ function PopoverReactionList(props) {
*/
const showReactionList = (event, reactionListAnchor, emojiName, reportActionID) => {
setReactionListReportActionID(reportActionID);
- innerReactionListRef.current.showReactionList(event, reactionListAnchor, emojiName);
+ setReactionListEmojiName(emojiName);
+ innerReactionListRef.current.showReactionList(event, reactionListAnchor);
};
useImperativeHandle(props.innerRef, () => ({showReactionList}), []);
@@ -33,6 +35,7 @@ function PopoverReactionList(props) {
<BasePopoverReactionList
ref={innerReactionListRef}
reportActionID={reactionListReportActionID}
+ emojiName={reactionListEmojiName}
/>
);
} |
…popover-reaction-list-not-update-dynamically
|
@s77rt Thanks for your detailed instructions.
For this, I suggest that we exclusively check the change for the selected emoji in Also, I notice that every time we scrolled up and down to bottom of the actions list, the parent |
s77rt
suggested changes
Aug 6, 2023
src/pages/home/report/ReactionList/PopoverReactionList/BasePopoverReactionList.js
Outdated
Show resolved
Hide resolved
src/pages/home/report/ReactionList/PopoverReactionList/index.js
Outdated
Show resolved
Hide resolved
|
@s77rt Resolved. |
Reviewer Checklist
Screenshots/VideosWebweb.mp4Mobile Web - Chromemweb-chrome.mp4Mobile Web - Safarimweb-safari.mp4Desktopdesktop.mp4iOSios.mp4Androidandroid.mp4 |
|
@tienifr I noticed the popover anchor position does not change when a reaction is added/removed dynamically. Was that the behaviour in the old implementation? |
|
cc @flodnv |
stitesExpensify
approved these changes
Aug 8, 2023
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
87 tasks
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.52-0 🚀
|
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.52-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Popover reaction list does not update dynamically on adding or removing reactions. This PR fixes that.
Fixed Issues
$ #23752
PROPOSAL: #23752 (comment)
Tests
========
Offline tests
NA
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
reaction-list-compressed.mov
Mobile Web - Chrome
video_2023-08-02_06-00-37.mp4
Mobile Web - Safari
Screen.Recording.2023-08-02.at.19.36.48.mov
Desktop
reaction-desktop-compressed.mov
iOS
Screen.Recording.2023-08-02.at.19.37.53.mov
Android
Screen.Recording.2023-08-02.at.19.38.53.mov