-
Notifications
You must be signed in to change notification settings - Fork 894
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
New core plugin for dynamic content rendering #7201
New core plugin for dynamic content rendering #7201
Conversation
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7201 +/- ##
==========================================
+ Coverage 67.53% 67.67% +0.14%
==========================================
Files 3504 3516 +12
Lines 69407 69544 +137
Branches 11324 11349 +25
==========================================
+ Hits 46872 47067 +195
+ Misses 19775 19692 -83
- Partials 2760 2785 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
db3ae33
to
ad9eb05
Compare
Signed-off-by: SuZhou-Joe <[email protected]>
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: Yulong Ruan <[email protected]>
c79d64d
to
95b6336
Compare
Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: Yulong Ruan <[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.
Overall no major concerns. Really good use of embeddable to simplify page building. One key callout from your original design that you missed here is the use of enums to remove the guess work from the content providers guessing the page section they want to contribute content to. i.e.
Create an enum for the content areas where the page is registered. e.g. in a file like src/plugins/home/public/content_areas.ts
we could define
export const HOME_CONTENT_AREAS = {
GET_STARTED: `${HOME_PAGE_ID}/get_started`,
SERVICE_CARDS: `${HOME_PAGE_ID}/service_cards`,
SOME_DASHBOARD: `${HOME_PAGE_ID}/some_dashboard`,
} as const;
export type HomeContentArea = typeof HOME_CONTENT_AREAS[keyof typeof HOME_CONTENT_AREAS];
Then make the content provider interface generic
export interface ContentProvider<T extends string = string> {
id: string;
getTargetArea: () => T;
getContent: () => Content;
}
and the registration function too
registerContentProvider: <T extends string = string>(provider: ContentProvider<T>) => void;
Then when a plugin registers content, it can just use this to correctly register content to the correct area:
import { HOME_CONTENT_AREAS, HomeContentArea } from '../content_areas';
contentManagement.registerContentProvider<HomeContentArea>({
id: 'home_get_start_1',
getTargetArea: () => HOME_CONTENT_AREAS.GET_STARTED,
getContent: () => ({
// ... content definition
}),
});
registerPage: ContentManagementService['registerPage']; | ||
} | ||
export interface ContentManagementPluginStart { | ||
registerContentProvider: (provider: ContentProvider) => void; |
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.
Cant this be in the setup method?
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.
At setup phase, the page and the sections will be registered, so at start phase, when register contents, the page and sections can be found. But essential, we could make it so that when registering contents, it doesn't rely on the existence of page and sections. I may leave as it is util there is a need to register content at setup phase.
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.
Ideally all setup should happen together. That's the benefit of the content management system. We can't move APIs around in a minor release. If we introduce it in the start phase, we can't move it to the setup phase until 3.0.
src/plugins/content_management/public/services/content_management/content_management_service.ts
Outdated
Show resolved
Hide resolved
src/plugins/content_management/public/components/card_container/card_container.tsx
Outdated
Show resolved
Hide resolved
title: section.title ?? 'test', | ||
query: { | ||
query: '', | ||
language: 'lucene', |
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.
is this the default language?
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 is the Todo part, actually this should be hooked up with the query input component on the page, but right now, there isn't a design for this, this is in the plan to improve in the future when requirement becomes clear
if (props.section.kind === 'card') { | ||
return <CardSection {...props} />; | ||
} |
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.
Whats an example of a card section?
Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: Yulong Ruan <[email protected]>
Signed-off-by: Yulong Ruan <[email protected]>
Checked that the failed cypress in |
* feat: add content management plugin Signed-off-by: Yulong Ruan <[email protected]> * fix: add license header Signed-off-by: Yulong Ruan <[email protected]> * add card container for rendering content as cards Signed-off-by: Yulong Ruan <[email protected]> * rename GetStartedCard -> CardList Signed-off-by: Yulong Ruan <[email protected]> * content management supporting embedding dashboards Signed-off-by: Yulong Ruan <[email protected]> * rename interface VisualizationInput -> SavedObjectInput Signed-off-by: Yulong Ruan <[email protected]> * cleanup and refactor Signed-off-by: Yulong Ruan <[email protected]> * fix license header Signed-off-by: Yulong Ruan <[email protected]> * add unit test for content management plugin Signed-off-by: Yulong Ruan <[email protected]> * Add a todo to cleanup demo code Signed-off-by: Yulong Ruan <[email protected]> * refactor: decouple content and page with content provider Signed-off-by: Yulong Ruan <[email protected]> * fix linter Signed-off-by: Yulong Ruan <[email protected]> * fix lint Signed-off-by: Yulong Ruan <[email protected]> * Changeset file for PR #7201 created/updated * pr feedback updates Signed-off-by: Yulong Ruan <[email protected]> * fix tests Signed-off-by: Yulong Ruan <[email protected]> * fix license header Signed-off-by: Yulong Ruan <[email protected]> * cleanup unused variable Signed-off-by: Yulong Ruan <[email protected]> * mark registerContentProvider API as experimental Signed-off-by: Yulong Ruan <[email protected]> --------- Signed-off-by: Yulong Ruan <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit cc6949b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* feat: add content management plugin * fix: add license header * add card container for rendering content as cards * rename GetStartedCard -> CardList * content management supporting embedding dashboards * rename interface VisualizationInput -> SavedObjectInput * cleanup and refactor * fix license header * add unit test for content management plugin * Add a todo to cleanup demo code * refactor: decouple content and page with content provider * fix linter * fix lint * Changeset file for PR #7201 created/updated * pr feedback updates * fix tests * fix license header * cleanup unused variable * mark registerContentProvider API as experimental --------- (cherry picked from commit cc6949b) Signed-off-by: Yulong Ruan <[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
This PR introduced a new mechanism for rendering dynamic pages for different purpose.
Find more details about this PR from this RFC #7228
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration