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.
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
Refactor OpenReport API call for Report GetHistory #10164
Refactor OpenReport API call for Report GetHistory #10164
Changes from 29 commits
713e1e0
de85a84
1c7bce4
8408423
7f11c94
b301b28
8260661
b654d97
5e37db3
2705f6e
2763fc4
9b1b0b6
3dc5dc6
c87a3e6
18d11f3
11cddbc
a298893
7468ac5
630f75c
d9909a2
df12dbd
a689a90
10b86ee
bc124fc
bca0f8b
39fd1ec
0be9f97
852e636
599ca8b
2d7de46
8954f03
8ebc4f7
d2695b2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How does the component get
this.props.report
now without usingwithOnyx()
?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.
from the parent component here:
App/src/pages/home/ReportScreen.js
Lines 242 to 245 in 972b835
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.
Oh, Thanks! I don't think we should pass both the
reportID
andreport
. Those are redundant. Let's just passreport
and get thereportID
off that.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.
I can do that, but reportID at the moment is
required
not sure if we should make thereport
required as well, not sure if we should have a default value for reportIDThere 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.
This sounds like an easy change and a good thing to look into. I also think it would be OK to do in a follow up and not add additional refactoring scope to these changes. I don't immediately see a clear reason for passing the
reportID
andreport
as separate props, but I haven't looked into it deeply enough to say there isn't one.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.
I think we should make
report
the required prop then, but as always, make sure you take a look at the code and understand it first before making a change like that.I won't fight tooth-and-nail about it being done in this PR, but I also think this is a refactor PR and I am not too concerned about scope for something small like this.
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.
just made the change and tested it after, seems to work great, let me know if you are happy with that @tgolen