-
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
[Workspace] Refactor the style of recent items card in the header #7805
[Workspace] Refactor the style of recent items card in the header #7805
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7805 +/- ##
==========================================
+ Coverage 63.83% 64.30% +0.47%
==========================================
Files 3658 3674 +16
Lines 81284 81166 -118
Branches 12972 12932 -40
==========================================
+ Hits 51884 52197 +313
+ Misses 26216 25756 -460
- Partials 3184 3213 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
c2b4652
to
451ff49
Compare
workspaceName?: string; | ||
}; | ||
|
||
export const bulkGetDetail = ( |
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.
What about move this into a utils file and rename to some others like bulkGetSavedObjectsDetail, which could better indicate the effect of this function. And we don't need to export if nowhere imports this.
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.
Thanks for the review, I delete export and I think it's better to keep it here as the required type SavedObjectWithMetadata has the import restriction. Here I just simply duplicate it in the file. [ Unexpected path "src/plugins/saved_objects_management/common/types" imported in restricted zone.]
6377112
to
8802d75
Compare
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
8802d75
to
d9d1ff6
Compare
})); | ||
|
||
if (savedObjects.length) { | ||
bulkGetDetail(savedObjects, http).then((res) => { |
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.
Why should it call this API to get the details? Isn't items from recentlyAccessed$
already have saved object title, type and workspace id?
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.
Hi, Thanks for your review, we call the API to get the associated icon
// Only display five most recent items | ||
return recentlyAccessedItems.slice(0, 5).map((item) => { | ||
return { | ||
link: createRecentNavLink(item, navLinks, basePath, navigateToUrl).href, |
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.
Seems we removed the createRecentNavLink import, but the link should be generated from it. It handles with the workspace id in path.
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.
Hi, thanks for your review,I delete it since we could directly access link from recentlyAccessedItems
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.
But that link is not correct as it does not contain workspace info.
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.
updated. Thank you
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
Signed-off-by: Qxisylolo <[email protected]>
savedObjects.map((obj) => | ||
http | ||
.get<SavedObjectWithMetadata>( | ||
`/api/opensearch-dashboards/management/saved_objects/${encodeURIComponent( |
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.
I'm not fully "comfortable" with calling a plugin API from a core module though the API is exposed by a core plugin.
But I've seen the saved object management API path /management/opensearch-dashboards
has already been used in multiple places, so I'm fine with it now.
In my opinion, perhaps this recent visited button should be owned by saved object management plugin because it's all about recently visited saved objects, I don't see a strong correlation between chrome module and the recently visited items, to my understanding, it's just another registration point that saved objects management plugin contributes to.
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.
Agree, things will look more in place if we can register
the recent items to the left of the breadcrumbs within saved objects management plugin.
) * feature/refractor-style-of-recent-items Signed-off-by: Qxisylolo <[email protected]> * solve bugs and delete annotation Signed-off-by: Qxisylolo <[email protected]> * solve bugs Signed-off-by: Qxisylolo <[email protected]> * fix bugs Signed-off-by: Qxisylolo <[email protected]> * fix bugs-1 Signed-off-by: Qxisylolo <[email protected]> * fix bugs-delete-lines Signed-off-by: Qxisylolo <[email protected]> * fix refine-popover-padding Signed-off-by: Qxisylolo <[email protected]> * fix, add createRecentNavLink Signed-off-by: Qxisylolo <[email protected]> * Changeset file for PR #7805 created/updated --------- Signed-off-by: Qxisylolo <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit a7024e6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
) (#7966) * feature/refractor-style-of-recent-items * solve bugs and delete annotation * solve bugs * fix bugs * fix bugs-1 * fix bugs-delete-lines * fix refine-popover-padding * fix, add createRecentNavLink * Changeset file for PR #7805 created/updated --------- (cherry picked from commit a7024e6) Signed-off-by: Qxisylolo <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
In this pr, I refactor the style of recent items card in the header by
Screenshot
Screen.Recording.2024-08-22.at.17.11.50.mov
empty state
Changelog
Check List
yarn test:jest
yarn test:jest_integration