-
Notifications
You must be signed in to change notification settings - Fork 919
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
Fix the display and jump logic for recent assets #8136
base: main
Are you sure you want to change the base?
Conversation
… to the global for workspace objects hwen workspace disabled Signed-off-by: Kapian1234 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8136 +/- ##
==========================================
+ Coverage 60.90% 60.98% +0.07%
==========================================
Files 3808 3812 +4
Lines 91196 91389 +193
Branches 14400 14439 +39
==========================================
+ Hits 55547 55731 +184
- Misses 32096 32098 +2
- Partials 3553 3560 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kapian1234 <[email protected]>
…boards into recent_assets
Signed-off-by: Kapian1234 <[email protected]>
get$: () => { | ||
return history.get$().pipe( | ||
map((items) => { | ||
return items.filter((item) => (workspaceEnabled ? !!item.workspaceId : true)); |
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.
Can we add some UT to test this filtering logic?
withoutClientBasePath: true, | ||
}) | ||
basePath.prepend( | ||
formatUrlWithWorkspaceId(link, workspaceEnabled ? workspaceId || '' : '', basePath), |
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.
This logic looks a little bit strange to me, I think we don't need to call formatUrlWithWorkspaceId
if workspaceEnabled equal false. How about refactor with below code?
formatUrlWithWorkspaceId(link, workspaceEnabled ? workspaceId || '' : '', basePath), | |
workspaceEnabled ? formatUrlWithWorkspaceId(link, workspaceId || '', basePath) : link, |
By the way, can we add some unit tests about it?
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.
sure
@@ -81,10 +84,16 @@ export class RecentlyAccessedService { | |||
}, | |||
|
|||
/** Gets the current array of history items. */ | |||
get: () => history.get(), | |||
get: () => history.get().filter((item) => !!item.workspaceId), |
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.
will this impact workspace disabled case? I think we should check workspaceEnabled flag
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.
yes I forgot it here, thx for reminding!
Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Kapian1234 <[email protected]>
@Kapian1234 thanks for the contribution. Could you check and fix the comment? I think we could release this for 2.19. Will leave @SuZhou-Joe and @ruanyl to make the call. |
ok |
Signed-off-by: Kapian1234 <[email protected]>
…boards into recent_assets
Signed-off-by: Kapian1234 <[email protected]>
@Kapian1234 , bootstrap failed, could you please take a look? |
Signed-off-by: Kapian1234 <[email protected]>
ok |
Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Kapian1234 <[email protected]>
@@ -103,6 +103,6 @@ describe('createRecentNavLink', () => { | |||
mockedNavigateToUrl | |||
); | |||
|
|||
expect(recentLink.href).toEqual('http://localhost/test/w/foo/app/foo'); | |||
expect(recentLink.href).toEqual('http://localhost/test/app/foo'); |
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.
Shall we need to add test case for workspace disabled?
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.
Added
Signed-off-by: Kapian1234 <[email protected]>
…boards into recent_assets
Description
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration