-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[No QA] refactor getReportName to have 1 param #83177
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -32,7 +32,8 @@ function TripChatNameEditPage({report}: TripChatNameEditPageProps) { | |
| const {inputCallbackRef} = useAutoFocusInput(); | ||
|
|
||
| const reportID = report?.reportID; | ||
| const currentChatName = getReportName(report); | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
|
Contributor
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. ❌ CONSISTENCY-5 (docs)This newly added Add a comment above or on the same line explaining the reason for the disable, for example: // This will be fixed as follow up https://github.com/Expensify/App/pull/75357
// eslint-disable-next-line @typescript-eslint/no-deprecated
const currentChatName = getReportName({report});Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Contributor
Author
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. Ignore lint check because we just need to refactor the getReportName function |
||
| const currentChatName = getReportName({report}); | ||
|
|
||
| const validate = (values: FormOnyxValues<typeof ONYXKEYS.FORMS.NEW_CHAT_NAME_FORM>): Errors => { | ||
| const errors: Errors = {}; | ||
|
|
||
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 would be really great if there were more explicit comments on the parameters in the type so people understand what their purpose is and when it's OK to pass it or not. Could you please add that for the two new parameters? Bonus points if you do it for all the parameters!
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.
In this PR, I just updated the GetReportNameParams type so I've added the comments for reportAttributes and conciergeReportID. Can you please check again?