Skip to content
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.

Sign up for GitHub

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 the componentDidUpdate method for ReportActionCompose #4768

Merged
merged 5 commits into from
Aug 30, 2021
Merged

Refactor the componentDidUpdate method for ReportActionCompose #4768

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
01d5c3d
Refactor the componentDidUpdate for ReportActionCompose
mountiny Aug 20, 2021
56dfcf0
Refactor and return early
mountiny Aug 25, 2021
57ec194
Refactor componentDidUpdate in ReportScreen as well and update comments
mountiny Aug 25, 2021
9eb84c0
Remove redundant comment
mountiny Aug 25, 2021
a27e869
Update the comment to give better description and reason for the change
mountiny Aug 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,13 @@ class ReportScreen extends React.Component {
}

componentDidUpdate(prevProps) {
const reportChanged = this.props.route.params.reportID !== prevProps.route.params.reportID;
if (reportChanged) {
this.prepareTransition();
this.storeCurrentlyViewedReport();
// If we do not switch between reports, return early
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment to @bondydaa but if a comment ends up just describing what the code is doing (or is very obvious) then we probably don't need the comment at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, in this case it is actually bit clearer why we check if the reports have changed. I will omit the comment here 👍

if (this.props.route.params.reportID === prevProps.route.params.reportID) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB, I know this was probably added since you noticed that the style doesn't match what I asked for in another comment. But since this change doesn't have anything to do with the current change I wonder if a better way to handle this would be to:

  1. Create a new issue calling out there there are places where early returns are not used
  2. Follow up to audit existing if blocks to see which are not using early returns and fix them

I'm not asking you to do it since we don't really have any rules or conventions about this. Some feel like it's fine to fix things that are wrong when you see them and others (like me) prefer to keep only material changes in PRs so that context is preserved and no one needs to ask why something was done. I don't think there is a "right way", so mostly just sharing this perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate you sharing this with me. This helps me a lot 🙇 I do agree it is not ideal to add changes and fixes to PR where it might be unrelated and uncalled for. I haven't realized it now, but I will definitely try to keep this on mind for future! It will make for clearer and easier to follow issues and PR for sure 🙌

I agree with you on the proposed process how to handle such a change, although as you stated, I think before creating such an investigation issue, we should make sure we update the Styling guidelines accordingly. That way if we fix this bad smell throughout the codebase we will have lesser chances of introducing it again.

Do you think this is work of being added to the styling guide, or it is too nitty-gritty 👀 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW how I view "adjacent style changes" (completely just made that up) like this are to weigh the "pros vs cons" of how bad it might be to have to revert or quickly fix if this breaks something unexpectedly.

I agree this seems like a pretty low risk change so probably fine to leave. But this mean you should be on the look out for any messages in #api-errors, #qa, or new GHs from Success that mention anything about weirdness about the report pages.

If you're not willing to be on "high alert" and fix something like that then I'd say only make the change you intended to make and leave the "adjacent style change" alone and spin up a new GH to fix it so that it's properly tested in it's own PR.

I don't think there is a need for a style guide rule or anything since there will be nuances to every PR where sometimes it makes sense to change all "adjacent style changes" and other times it might not make the most sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bondydaa Thank you very much for your 2 cents here! Glad to have another perspective on this 💯

I will keep an eye on those channels! Thanks for the heads up. I think in this case there should not be high risk 🤞 since the logic stays the same, but I totally see what you mean. Even with minor styling changes, we should make sure all the flows are working and no edge case has been omitted 👍

Regarding the styling guide, I see your point. Eventually, informing more and more engineers about this style approach, we might get more of them using it in their code and PR reviews consequently making it standard the organic way.

}

this.prepareTransition();
this.storeCurrentlyViewedReport();
}

componentWillUnmount() {
Expand Down
9 changes: 5 additions & 4 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,11 +189,12 @@ class ReportActionCompose extends React.Component {
this.focus();
}

// If we switch from a sidebar, the component does not mount again
// so we need to update the comment manually.
if (prevProps.comment !== this.props.comment) {
this.textInput.setNativeProps({text: this.props.comment});
// If we switch the reports, make sure to update the composer comment; otherwise return early
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB comments should explain why not what we are doing in the code. so I would suggest removing the otherwise return early and instead a because... explaining why we need to update the composer comment:

Suggested change
// If we switch the reports, make sure to update the composer comment; otherwise return early
// If we switch the reports, make sure to update the composer comment because [we don't want to leave old data around] [we shouldn't copy what a user had types in one place to another]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I was not sure about that comment actually, thank you for bringing it up! I will update it just now.

if (this.props.report.reportID === prevProps.report.reportID) {
return;
}

this.updateComment(this.props.comment);
}

componentWillUnmount() {
Expand Down