Conversation
… the scroll animation on focus
melvin-bot
bot
requested review from
srikarparsi
and removed request for
a team
|
@srikarparsi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
…2-Unable-to-edit-message # Conflicts: # src/pages/home/report/ReportActionItemMessageEdit.js # src/pages/home/report/ReportActionsList.js
| shouldFreezeScroll: PropTypes.bool, | ||
|
|
||
| /** Function to update the state */ | ||
| setShouldFreezeScroll: PropTypes.func, |
There was a problem hiding this comment.
| shouldFreezeScroll: PropTypes.bool, | |
| /** Function to update the state */ | |
| setShouldFreezeScroll: PropTypes.func, | |
| shouldFreezeScroll: PropTypes.bool.isRequired, | |
| /** Function to update the state */ | |
| setShouldFreezeScroll: PropTypes.func.isRequired, |
| ); | ||
|
|
||
| return <ReportActionListFrozenScrollContext.Provider | ||
| value={contextValue}>{props.children}</ReportActionListFrozenScrollContext.Provider>; |
There was a problem hiding this comment.
The useMemo looks like an overhead here.
cc @fabioh8010 @gedu
| value={contextValue}>{props.children}</ReportActionListFrozenScrollContext.Provider>; | |
| value={{ | |
| shouldFreezeScroll, | |
| setShouldFreezeScroll, | |
| }}>{props.children}</ReportActionListFrozenScrollContext.Provider>; |
There was a problem hiding this comment.
I think it's correct @rezkiy37, because if we pass an object in this way, is like we are creating a new object every time and it will trigger unnecessary re-renders (old object will never equal newly created one). It makes sense to me to memoize.
There was a problem hiding this comment.
It is ok to memo. it is a common practice to always memo the objects from Context
There was a problem hiding this comment.
This does not need to be memoized. We could return the useStateobject to the provider (as an array, not object), which will be the same reference on every render. WDYT?
function ReportActionListFrozenScrollContextProvider(props) {
const shouldFreezeScrollState = useState(false);
return (
<ReportActionListFrozenScrollContext.Provider value={shouldFreezeScrollState}>
{props.children}
</ReportActionListFrozenScrollContext.Provider>
);
}
rezkiy37
left a comment
rezkiy37
left a comment
There was a problem hiding this comment.
Left a couple comments. In general, LGTM.
| ); | ||
|
|
||
| return <ReportActionListFrozenScrollContext.Provider | ||
| value={contextValue}>{props.children}</ReportActionListFrozenScrollContext.Provider>; |
There was a problem hiding this comment.
I think it's correct @rezkiy37, because if we pass an object in this way, is like we are creating a new object every time and it will trigger unnecessary re-renders (old object will never equal newly created one). It makes sense to me to memoize.
| children: PropTypes.node.isRequired, | ||
| }; | ||
|
|
||
| function withScrollFrozen(WrappedComponent) { |
There was a problem hiding this comment.
Why are we creating this HOC? It's not being used anywhere.
| * @returns {Object} | ||
| */ | ||
| export default function useFrozenScroll() { | ||
| return useContext(ReportActionListFrozenScrollContext); |
There was a problem hiding this comment.
We could improve error handling here.
| return useContext(ReportActionListFrozenScrollContext); | |
| const context = useContext(ReportActionListFrozenScrollContext); | |
| if (context === undefined) | |
| throw new Error('useFrozenScroll must be used within ReportActionListFrozenScrollContextProvider'); | |
| } | |
| return context; |
Santhosh-Sellavel
left a comment
Santhosh-Sellavel
left a comment
There was a problem hiding this comment.
Resolve conflicts. Adding myself as a reviewer
…2-Unable-to-edit-message # Conflicts: # src/pages/home/ReportScreen.js # src/pages/home/report/ReportActionItemMessageEdit.js # src/pages/home/report/ReportActionsList.js
|
@Santhosh-Sellavel conflicts are resolved |
|
Still conflicts |
…2-Unable-to-edit-message # Conflicts: # src/pages/home/report/ReportActionItemMessageEdit.js
|
@Santhosh-Sellavel conflicts resolved, looks like in the middle of pr there were changes again in one of the files |
|
This ran into conflicts again. |
…2-Unable-to-edit-message # Conflicts: # src/pages/home/ReportScreen.js
|
@Santhosh-Sellavel looks like there was refactoring meanwhile to functional component, conflicts resolved |
|
@Piotrfj Please add recordings |
|
@Piotrfj Update what to test/verify on different platforms. |
…2-Unable-to-edit-message
|
@marcochavezf done :') |
|
Oh now lint errors 😌 |
…2-Unable-to-edit-message
|
@marcochavezf done :) |
|
@Piotrfj Still lint failing! |
|
Tests fails And Glitch while running test steps on a new thread/chat. Definitely Deploy blockers it could affect new users experience. Screen.Recording.2023-09-26.at.1.21.53.AM.mov |
Santhosh-Sellavel
left a comment
Santhosh-Sellavel
left a comment
There was a problem hiding this comment.
Bug: #24154 (comment)
|
He is on sick leave, he will be back soon |
|
@Santhosh-Sellavel looking at it today |
|
Bump @Piotrfj |
1 similar comment
|
Bump @Piotrfj |
|
@Santhosh-Sellavel There is a difference between safari and other environments in behavior I havent discovered before, currently Im working on the solution that would take care of that and will not affect other environments |
|
I remember this affects all Mweb not just Safari, please check again & confirm. |
|
Can resolve conflicts please? |
|
@Santhosh-Sellavel , hey @Piotrfj is on sick leave. He should be back tomorrow. |
|
@Santhosh-Sellavel just came back from sick leave, going back to work on the issue, will give some feedback today. |
|
Any update @Piotrfj? |
|
@Santhosh-Sellavel I have put the comment on different task by mistake, first solution was buggy and right now I'm close to complete second solution, but still I'm encountering problems with safari. Will give some more feedback tomorrow as I will discuss this with inner teammates. |
|
@Santhosh-Sellavel After all this battle and consulting I haven't been able to provide well working solution. However we have found that the issue is in progres on react-native-web library. So we are suggesting to put our issue ON HOLD until the problem is resolved at source or merge current solution (only safari is not working) and open new issue just for safari. |
|
@Santhosh-Sellavel necolas/react-native-web#2588 thats the issue I was talking about. |
|
Hey! Please re-assign me when this PR is ready for review :) |
|
Can you resolve conflicts, let's test this again to confirm this.
|
|
It's been a year without activity, I will close it out to remove it from the k2. However, feel free to re-open if there's something we need to look up |
10 participants
Details
Fixed Issues
$ #18805
PROPOSAL: #18805 (comment)
Tests
Scenario 1:
Scenario 2:
Offline tests
Bug is only testable on online mode since we are testing behavior when new messages are appearing.
QA Steps
Scenario 1:
Scenario 2:
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
Kapture.2023-08-30.at.23.43.00.mp4
Android
Kapture.2023-08-30.at.23.14.54.mp4