-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix editing the message #24154
Fix editing the message #24154
Changes from 15 commits
78b9f1d
85b5450
9ff6e31
45e4736
7cb0e91
747b378
739a7d0
23b8e40
7936593
3a93dd3
ce13894
592ecf3
292a4a6
bd95a39
66ded3b
ae3fab6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import {useContext} from 'react'; | ||
import {ReportActionListFrozenScrollContext} from "../pages/home/report/ReportActionListFrozenScrollContext"; | ||
|
||
/** | ||
* Hook for getting current state of scroll freeze and a function to set whether the scroll should be frozen | ||
* @returns {Object} | ||
*/ | ||
export default function useFrozenScroll() { | ||
return useContext(ReportActionListFrozenScrollContext); | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,67 @@ | ||||||||||||||||||
import React, {createContext, forwardRef, useMemo, useState} from "react"; | ||||||||||||||||||
import PropTypes from "prop-types"; | ||||||||||||||||||
import getComponentDisplayName from "../../../libs/getComponentDisplayName"; | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
const withScrollFrozenPropTypes = { | ||||||||||||||||||
/** flag determining if we should freeze the scroll */ | ||||||||||||||||||
shouldFreezeScroll: PropTypes.bool, | ||||||||||||||||||
|
||||||||||||||||||
/** Function to update the state */ | ||||||||||||||||||
setShouldFreezeScroll: PropTypes.func, | ||||||||||||||||||
Comment on lines
+7
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
const ReportActionListFrozenScrollContext = createContext(null); | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
function ReportActionListFrozenScrollContextProvider(props) { | ||||||||||||||||||
const [shouldFreezeScroll, setShouldFreezeScroll] = useState(false); | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* The context this component exposes to child components | ||||||||||||||||||
* @returns {Object} flag and a flag setter | ||||||||||||||||||
*/ | ||||||||||||||||||
const contextValue = useMemo( | ||||||||||||||||||
() => ({ | ||||||||||||||||||
shouldFreezeScroll, | ||||||||||||||||||
setShouldFreezeScroll, | ||||||||||||||||||
}), | ||||||||||||||||||
[shouldFreezeScroll, setShouldFreezeScroll], | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
return <ReportActionListFrozenScrollContext.Provider | ||||||||||||||||||
value={contextValue}>{props.children}</ReportActionListFrozenScrollContext.Provider>; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is ok to memo. it is a common practice to always memo the objects from Context There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not need to be memoized. We could return the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with @ArekChr's approach. |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
ReportActionListFrozenScrollContextProvider.displayName = 'ReportActionListFrozenScrollContextProvider'; | ||||||||||||||||||
ReportActionListFrozenScrollContextProvider.propTypes = { | ||||||||||||||||||
/** Actual content wrapped by this component */ | ||||||||||||||||||
children: PropTypes.node.isRequired, | ||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
function withScrollFrozen(WrappedComponent) { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we creating this HOC? It's not being used anywhere. |
||||||||||||||||||
const WithScrollFrozenState = forwardRef((props, ref) => ( | ||||||||||||||||||
<ReportActionListFrozenScrollContext.Consumer> | ||||||||||||||||||
{(scrollFrozenProps) => | ||||||||||||||||||
<WrappedComponent | ||||||||||||||||||
// eslint-disable-next-line react/jsx-props-no-spreading | ||||||||||||||||||
{...scrollFrozenProps} | ||||||||||||||||||
// eslint-disable-next-line react/jsx-props-no-spreading | ||||||||||||||||||
{...props} | ||||||||||||||||||
ref={ref} | ||||||||||||||||||
/> | ||||||||||||||||||
} | ||||||||||||||||||
</ReportActionListFrozenScrollContext.Consumer> | ||||||||||||||||||
)); | ||||||||||||||||||
|
||||||||||||||||||
WithScrollFrozenState.displayName = `WithScrollFrozenState(${getComponentDisplayName(WrappedComponent)})`; | ||||||||||||||||||
return WithScrollFrozenState; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
export { | ||||||||||||||||||
ReportActionListFrozenScrollContext, | ||||||||||||||||||
ReportActionListFrozenScrollContextProvider, | ||||||||||||||||||
withScrollFrozenPropTypes, | ||||||||||||||||||
withScrollFrozen | ||||||||||||||||||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could improve error handling here.