-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add HOC withParentReportAction
#18638
Conversation
withParentReportAction
@marcochavezf 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] |
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.
It feels like this HOC should be created and implemented at least in one place. Otherwise its kinda hard to see its benefit.
@chiragsalian I agree, let's hold on https://github.com/Expensify/App/pull/18522/files |
withParentReportAction
withParentReportAction
withParentReportAction
withParentReportAction
withParentReportAction
withParentReportAction
will update this week, just lower priority (refactor) than other thread bugs |
@grgia When you edit the parent report action from the thread, the edit input appears. However, when you save it, the parent report action is not updated, unlike when you update it from the parent report. I think we should either fix this issue or disable the ability to edit the parent report action from the thread. Screen.Recording.2023-06-08.at.9.33.37.AM.mov |
@fedirjh is this reproducible on main? |
Dropping as a todo: need to use here too #20273 |
withParentReportAction
withParentReportAction
Yes , the update comment request is made to server however the component doesn’t re-render. Bug.mov |
withParentReportAction
withParentReportAction
Wa have a failing lint test . should run prettier |
@fedirjh done, thank you! |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-06-15.at.12.22.31.PM.movMobile Web - ChromeScreen.Recording.2023-06-15.at.1.14.00.PM.movMobile Web - SafariScreen.Recording.2023-06-15.at.12.53.35.PM.movDesktopScreen.Recording.2023-06-15.at.12.42.48.PM.movScreen.Recording.2023-06-15.at.12.42.17.PM.moviOSScreen.Recording.2023-06-15.at.12.52.57.PM.movAndroidScreen.Recording.2023-06-15.at.1.12.27.PM.mov |
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.
Looks good and tests well , we still have this issue but it's not related to this PR
We did not find an internal engineer to review this PR, trying to assign a random engineer to #18755... Please reach out for help on Slack if no one gets assigned! |
@fedirjh merged main |
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.
LGTM and tests well
🎀 👀 🎀 C+ reviewed
We did not find an internal engineer to review this PR, trying to assign a random engineer to #18755... Please reach out for help on Slack if no one gets assigned! |
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.
LGTM, just a minor comment
parentReportActions: {}, | ||
}; | ||
function WithParentReportAction(props) { | ||
const parentReportActionID = props.report && props.report.parentReportActionID ? props.report.parentReportActionID : {}; |
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.
Ah why are we setting an empty object for parentReportActionID
? Additionally, shouldn't we add parentReportActionID
to reportPropTypes
?
I am concerned if this PR is needed tbh. I would rather see a proper long-term solution i.e., using @marcaaron since you are more familiar with |
@chiragsalian I agree with that solution, we attempted a selector pattern with Onyx, but couldn't get it working with dependencies from other onyx props. For example, needing to subscribe to a certain report you're referencing to get the parentReportActionID from. You'd initialize both withOnyx values at the same time, even though one depends on the other (which was what was breaking the selector when we were attempting to get that to work) |
So I guess my question is, since there is no rush to get this feature out the door why not focus on a solution that can be reused in multiple places? Do we feel like its not worth focusing on a more scalable solution now? |
Ok so I see the behavior we want is to get a particular report action that belongs to the My suggestion would be to:
parentReportAction: {
key: ({report}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`,
selector: (parentReportActions, props) => parentReportActions[props.report.parentReportActionID],
}, I think something like this would work and maybe a bit simpler than creating a new HOC. |
Checking in to see if anyone had thoughts on the comment I shared above? Thanks! |
@marcaaron I apologize for the delay, I agree and I will close this PR. I think we need to add the selector. So long as we pass the entire report down, that should work |
As a note, I think we should work on that approach in a separate issue as it will require ONYX changes since we could not originally get passing props to the selector to work when we were in the first phase of implementation @marcaaron |
Details
Create a HOC so we can subscribe to the parent report action.
Uses same pattern as for
useCurrentUserPersonalDetails
Fixed Issues
#18755
$ #18769
Tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
N/A
Mobile Web - Safari
N/A
Desktop
N/A
iOS
N/A
Android
N/A