-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Update Workspaces tab URLs to just workspaces to clearly differentiate them from Account-related URLs #65018
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
8d84f89
39d6bb7
7fb6b3f
e599ae8
a5a201a
a3842d1
7d27062
2ff839c
3af0a69
2472456
6b7a6ee
08ab6fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Large diffs are not rendered by default.
|
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. @sumo-slonik These changes feel to a big part unrelated, why is it here?
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. This file is automatically generated. I regenerated it to include the changes I made, using the following command:
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. yeahhh, we need this help content, for example, the members page content is not present on staging/ prod, but with our PR, we add the content and now we can see the same when we visit the members page, Now i also feel like @blazejkustra should review this PR once, since they were the implementor of the help content PR.
Makes sense! Staging:
Our PR:
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.
That's exactly what happened, Ted added some content here but didn't regenerate
I was overseeing this PR in terms of the help panel, and everything was implemented according to the current standards @allgandalf @mountiny
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. Now that I think about it I believe it would be safer to wait for this to be merged first, then regenerate content one more time here and then merge this PR |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import oldRoutes from '@navigation/linkingConfig/OldRoutes'; | ||
|
|
||
| /** | ||
| * Maps an old route path to its corresponding new route based on the `oldRoutes` map. | ||
| * It finds the best matching pattern (with wildcard `*` support) and replaces the matched | ||
| * part of the path with the new route value. | ||
| * | ||
| * @param path - The input URL path to match and transform. | ||
| * @returns The new route path if a match is found, otherwise `undefined`. | ||
| * | ||
| * Related issue: https://github.com/Expensify/App/issues/64968 | ||
| */ | ||
| function getMatchingNewRoute(path: string) { | ||
| let bestMatch; | ||
| let maxLength = -1; | ||
|
|
||
| for (const pattern of Object.keys(oldRoutes)) { | ||
| const regexStr = `^${pattern.replace('*', '.*')}`; | ||
| const regex = new RegExp(regexStr); | ||
|
|
||
| if (regex.test(path) && pattern.length > maxLength) { | ||
| bestMatch = pattern; | ||
| maxLength = pattern.length; | ||
| } | ||
| } | ||
| if (!bestMatch) { | ||
| return bestMatch; | ||
| } | ||
|
|
||
| const finalRegexp = bestMatch?.replace('*', '') ?? ''; | ||
| return path.replace(finalRegexp, oldRoutes[bestMatch]); | ||
| } | ||
| export default getMatchingNewRoute; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,8 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const oldRoutes: Record<string, string> = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. I feel like we could add this to cloudfront to redirect, but also do we need to add /workspaces to the AndroidManifest or apple-site json thing for deeplinks? App/android/app/src/main/AndroidManifest.xml Lines 61 to 106 in 3caba34
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. I'll finish the changes in the PR related to the empty state in the report, and then I'll get started on this.
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. @sumo-slonik ok there are some conflicts now, can you please check the deeplink configs for ios and android? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '/settings/workspaces/*': '/workspaces/', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '/settings/workspaces': '/workspaces', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export default oldRoutes; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import getMatchingNewRoute from '@navigation/helpers/getMatchingNewRoute'; | ||
|
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. nice job with the tests! |
||
|
|
||
| describe('getBestMatchingPath', () => { | ||
| it('returns mapped base path when input matches the exact pattern', () => { | ||
| expect(getMatchingNewRoute('/settings/workspaces/')).toBe('/workspaces/'); | ||
| }); | ||
|
|
||
| it('returns mapped base path when input matches the exact pattern', () => { | ||
| expect(getMatchingNewRoute('/settings/workspaces')).toBe('/workspaces'); | ||
| }); | ||
|
|
||
| it('returns mapped path when input matches the pattern and have more content', () => { | ||
| expect(getMatchingNewRoute('/settings/workspaces/anything/more')).toBe('/workspaces/anything/more'); | ||
| }); | ||
|
|
||
| it('returns undefined when input does not match any pattern - similar prefix but different ending', () => { | ||
| expect(getMatchingNewRoute('/settings/anything/')).toBe(undefined); | ||
| }); | ||
|
|
||
| it('returns undefined when input is unrelated to any pattern', () => { | ||
| expect(getMatchingNewRoute('/anything/workspaces/')).toBe(undefined); | ||
| expect(getMatchingNewRoute('/anything/anything/')).toBe(undefined); | ||
| expect(getMatchingNewRoute('/anything/anything/anything')).toBe(undefined); | ||
| }); | ||
| }); | ||


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.
We should also add this new path to apple-app-site-association
file, so our smart app banner can display on mSafari.
More details:
#66743 (comment)