-
Notifications
You must be signed in to change notification settings - Fork 52
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
[Feature] OverviewPage made with Content Management #2077
Conversation
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Dan Dong <[email protected]>
Signed-off-by: Dan Dong <[email protected]>
df4fb26
to
1a469cd
Compare
Signed-off-by: Dan Dong <[email protected]>
Signed-off-by: Dan Dong <[email protected]>
1a469cd
to
5c578a4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2077 +/- ##
==========================================
- Coverage 57.85% 54.81% -3.04%
==========================================
Files 372 399 +27
Lines 14081 15306 +1225
Branches 3712 4134 +422
==========================================
+ Hits 8146 8390 +244
- Misses 5871 6835 +964
- Partials 64 81 +17
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: Dan Dong <[email protected]>
…ets user setting Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
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 @TackAdam , thanks for putting this together. In generally this is looking good to me=, and I just left some comments.
|
||
useEffect(() => { | ||
coreRefs.savedObjectsClient | ||
?.find({ |
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.
Instead of repeatedly accessing properties, can we destructure the response at the beginning?
useEffect(() => {
const fetchDashboards = async () => {
const response = await coreRefs.savedObjectsClient?.find({ type: 'dashboard' });
const savedDashboards = response?.savedObjects.reduce((acc, { id, attributes }) => {
const { title, timeFrom, timeTo } = attributes as {
title: string;
timeFrom: string;
timeTo: string;
};
acc[id] = {
value: id,
label: title,
startDate: timeFrom,
endDate: timeTo,
};
return acc;
}, {} as DashboardDictionary);
setDashboards(savedDashboards || {});
const defaultDashboard = uiSettingsService.get(uiSettingsKey);
if (defaultDashboard && savedDashboards?.[defaultDashboard]) {
const { label, startDate, endDate } = savedDashboards[defaultDashboard];
setDashboardTitle(label);
setDashboardId(defaultDashboard);
setStartDate(startDate);
setEndDate(endDate);
wrapper.dashboardSelected = true;
} else {
uiSettingsService.set('observability:defaultDashboard', null);
setDashboardTitle('');
setDashboardId('');
wrapper.dashboardSelected = false;
}
};
fetchDashboards();
}, []);
Also in the above comment, since we have destructured the attribute
we do not need the toString()
for id:
// Original
const dashboardAttributes = savedDashboard.attributes as { title: string; timeFrom: string; timeTo: string };
const id = savedDashboard.id.toString();
// Refactored
const { title, timeFrom, timeTo } = attributes as { title: string; timeFrom: string; timeTo: string };
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.
Good call out, will look to implement this in a later PR when time allows thorough testing.
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.
➕ to Ryan, Let's try to keep the useEffect cleaner, may be move this to function call and function can be declared outside useEffect
Signed-off-by: Adam Tackett <[email protected]>
public/components/overview/components/select_dashboard_flyout.tsx
Outdated
Show resolved
Hide resolved
public/components/overview/components/select_dashboard_flyout.tsx
Outdated
Show resolved
Hide resolved
public/components/overview/home.tsx
Outdated
coreRefs?.application!.navigateToApp(appId, { | ||
path: `${path}`, | ||
}); | ||
}; | ||
|
||
export type AppAnalyticsCoreDeps = TraceAnalyticsCoreDeps; | ||
coreRefs.contentManagement?.registerContentProvider({ |
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 is this declared outside home? If this needs to be registered just once outside of react page updates then this should be part of plugin setup or plugin start.
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.
Pending Fix in follow up PR / content management changes.
Signed-off-by: Shenoy Pratik <[email protected]>
Signed-off-by: Shenoy Pratik <[email protected]>
Signed-off-by: Shenoy Pratik <[email protected]>
Signed-off-by: Shenoy Pratik <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
registerCards(); | ||
registerDashboard(); | ||
registerDashboardsControl(); | ||
loadHomePage(); |
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.
Since all of the functions here are asynchronous, could there be any race conditions? Would any of these processes be dependent on any of the others in any way?
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.
No each is independent. The registration for each section between the cards / dashboards / controls should be non reliant on each other. Haven't encountered any issues in testing.
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/dashboards-observability/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/dashboards-observability/backport-2.x
# Create a new branch
git switch --create backport/backport-2077-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 881e56ac3742e716d7b25458aa75d716e7c83038
# Push it to GitHub
git push --set-upstream origin backport/backport-2077-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/dashboards-observability/backport-2.x Then, create a pull request where the |
…ct#2077) * overview page with content management Signed-off-by: Adam Tackett <[email protected]> * Changed dashboard picker modal to be flyout Signed-off-by: Dan Dong <[email protected]> * Changed cards to be scrollable Signed-off-by: Dan Dong <[email protected]> * Deep linked dashboard from overview page Signed-off-by: Dan Dong <[email protected]> * Updated jest testing Signed-off-by: Dan Dong <[email protected]> * Fixed bugs Signed-off-by: Dan Dong <[email protected]> * validates the dashboard exsit before loading, clears if it is and resets user setting Signed-off-by: Adam Tackett <[email protected]> * Add Observability Overview breadcrumb to display title Signed-off-by: Adam Tackett <[email protected]> * Update snapshots, small ui changes Signed-off-by: Adam Tackett <[email protected]> * Ui adjustments, moved header wrapper to helper folder Signed-off-by: Adam Tackett <[email protected]> * fix linting error Signed-off-by: Adam Tackett <[email protected]> * remove comments Signed-off-by: Adam Tackett <[email protected]> * adding dashboard manager for state mgmt Signed-off-by: Shenoy Pratik <[email protected]> * refine observables Signed-off-by: Shenoy Pratik <[email protected]> * update tests and rename components Signed-off-by: Shenoy Pratik <[email protected]> * add comment to move away from rxjs Signed-off-by: Shenoy Pratik <[email protected]> * searchable selectable box added for dashboard selection Signed-off-by: Adam Tackett <[email protected]> * updated shared variables, update text for cards Signed-off-by: Adam Tackett <[email protected]> * rename variables Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Signed-off-by: Dan Dong <[email protected]> Signed-off-by: Adam Tackett <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Co-authored-by: Adam Tackett <[email protected]> Co-authored-by: Dan Dong <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]> (cherry picked from commit 881e56a)
* overview page with content management Signed-off-by: Adam Tackett <[email protected]> * Changed dashboard picker modal to be flyout Signed-off-by: Dan Dong <[email protected]> * Changed cards to be scrollable Signed-off-by: Dan Dong <[email protected]> * Deep linked dashboard from overview page Signed-off-by: Dan Dong <[email protected]> * Updated jest testing Signed-off-by: Dan Dong <[email protected]> * Fixed bugs Signed-off-by: Dan Dong <[email protected]> * validates the dashboard exsit before loading, clears if it is and resets user setting Signed-off-by: Adam Tackett <[email protected]> * Add Observability Overview breadcrumb to display title Signed-off-by: Adam Tackett <[email protected]> * Update snapshots, small ui changes Signed-off-by: Adam Tackett <[email protected]> * Ui adjustments, moved header wrapper to helper folder Signed-off-by: Adam Tackett <[email protected]> * fix linting error Signed-off-by: Adam Tackett <[email protected]> * remove comments Signed-off-by: Adam Tackett <[email protected]> * adding dashboard manager for state mgmt Signed-off-by: Shenoy Pratik <[email protected]> * refine observables Signed-off-by: Shenoy Pratik <[email protected]> * update tests and rename components Signed-off-by: Shenoy Pratik <[email protected]> * add comment to move away from rxjs Signed-off-by: Shenoy Pratik <[email protected]> * searchable selectable box added for dashboard selection Signed-off-by: Adam Tackett <[email protected]> * updated shared variables, update text for cards Signed-off-by: Adam Tackett <[email protected]> * rename variables Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Signed-off-by: Dan Dong <[email protected]> Signed-off-by: Adam Tackett <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Co-authored-by: Adam Tackett <[email protected]> Co-authored-by: Dan Dong <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]> (cherry picked from commit 881e56a)
Description
Changes to overview page:
Before:
After:
New setting:
Getting Started before:
After:
Issues Resolved
New standard for Overview pages from opensearch-project/OpenSearch-Dashboards#7201
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.