Replace all instances of "report" in Publisher #173
Conversation
| Reports = "REPORTS", | ||
| CreateReport = "CREATE REPORT", | ||
| Reports = "RECORDS", | ||
| CreateReport = "CREATE RECORD", |
There was a problem hiding this comment.
TypeScript is strict about enums not having computed values - only strings, so I manually updated these.
| @@ -83,7 +83,7 @@ test("displayed created reports", async () => { | |||
| }); | |||
| }); | |||
|
|
|||
| const annualReport2020 = screen.getByText(/Annual Report FY2020-2021/i); | |||
| const annualReport2020 = screen.getByText(/Annual Record FY2020-2021/i); | |||
There was a problem hiding this comment.
Couldn't do a computed value inside of a regex, so I opted to manually update this one.
mxosman
changed the title
There was a problem hiding this comment.
Awesome! Thanks for going through the whole app!
I'm wondering if it would be worth renaming all our constants / file names to Record instead of Report, so that we don't refer to Report internally but have to remember to say Record externally, or new developers have to learn that Report = Record?
Or maybe it's not worth such a big constant renaming push especially if the name changes again?
| export const REPORT_PERIOD_CAPITALIZED = "Time Period"; | ||
|
|
||
| export const REPORT_LOWERCASE = "record"; | ||
| export const REPORT_LOWERCASE2 = "share"; |
There was a problem hiding this comment.
What's the difference between REPORT_LOWERCASE2, REPORT_VERB_LOWERCASE, GATHER_LOWERCASE? Since they are all defined as "share"?
There was a problem hiding this comment.
Should we consolidate these constants, like just reuse the share constant instead of create one for GATHER?
There was a problem hiding this comment.
Yea I agree that REPORT_LOWERCASE and REPORT_VERB_LOWERCASE can probably be consolidated? And REPORT_LOWERCASE3 should maybe be REPORT_VERB_LOWERCASE2, and consolidated with GATHER_LOWERCASE?
There was a problem hiding this comment.
Great call! Was super tunnel-visioned here - will consolidate this.
There was a problem hiding this comment.
Cleaned up here: d418ae6
@terryttsai this is a good callout -- @mxosman and I discussed this and I actually suggested keeping the code as Report. It does seem likely/possible that it will change again, and I think it's okay for our code and our internal mental model to stay with "Report". I don't feel strongly and I see the argument both ways, but given how much easier it is to keep as-is, that would be my suggestion. |
There was a problem hiding this comment.
I went through ~60 files that had the word "report" in it, and found some hidden hotspots like toasts/error messages, etc.
@mxosman that's amazing -- thank you SO MUCH for doing such impressive due diligence!
Yeah - this was also more/less the quickest solution to unblock Humphrey. I think there is somewhere around 1k instances of "report" to replace - probably best to feel confident that it won't change before fully switching over. |
|
Keeping the internal mental model as Report sounds good to me, thanks for explaining the thought process! |
I do think that eventually (once the naming is stable) it would be worth it to rename everything for all the reasons you mentioned! |
Update all instances of report to the respective constant Update readme Fix lint and tests Spacing issue Fix spacing Consolidate constants
mxosman
force-pushed
the
mahmoud/replace-report-text
branch
from
d418ae6
to
70ef73e
Compare
Description of the change
Replaces all user-facing instances of the word "report" (as outlined in this doc) with relevant replacement constants. There were other components outside of the ones listed in the doc that needed updating - I went through ~60 files that had the word "report" in it, and found some hidden hotspots like toasts/error messages, etc.
If you have a moment, please feel free to test the link below and let me know if there are areas I may have missed!
Demo: https://mahmoudtest---justice-counts-web-qqec6jbn6a-uc.a.run.app/
Related issues
Closes #165
Checklists
Development
This box MUST be checked by the submitter prior to merging:
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: