Skip to content
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

Conversation

ruanyl
Copy link
Member

@ruanyl ruanyl commented Jul 9, 2024

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

  • feat: introduced an new plugin contentManagement for dynamic content rendering

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

github-actions bot commented Jul 9, 2024

❌ Empty Changelog Section

The 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.

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 79.09836% with 51 lines in your changes missing coverage. Please review.

Project coverage is 67.67%. Comparing base (6f28170) to head (53d31da).

Files Patch % Lines
src/plugins/content_management/public/plugin.ts 0.00% 12 Missing ⚠️
...tent_management/public/components/section_input.ts 86.76% 3 Missing and 6 partials ⚠️
...nt_management/public/components/section_render.tsx 84.37% 2 Missing and 3 partials ⚠️
...s/content_management/content_management_service.ts 80.76% 3 Missing and 2 partials ⚠️
...agement/public/services/content_management/page.ts 80.00% 1 Missing and 4 partials ⚠️
...blic/components/card_container/card_embeddable.tsx 66.66% 1 Missing and 3 partials ⚠️
...ublic/components/card_container/card_container.tsx 72.72% 1 Missing and 2 partials ⚠️
...nt/public/components/custom_content_embeddable.tsx 72.72% 1 Missing and 2 partials ⚠️
src/plugins/content_management/public/app.tsx 50.00% 1 Missing ⚠️
src/plugins/content_management/public/index.ts 50.00% 1 Missing ⚠️
... and 3 more
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     
Flag Coverage Δ
Linux_1 33.15% <25.00%> (-0.01%) ⬇️
Linux_2 55.46% <ø> (ø)
Linux_3 43.31% <80.63%> (+0.25%) ⬆️
Linux_4 34.69% <12.29%> (-0.08%) ⬇️
Windows_1 33.17% <25.00%> (-0.01%) ⬇️
Windows_2 55.41% <ø> (ø)
Windows_3 43.32% <80.63%> (+0.25%) ⬆️
Windows_4 34.69% <12.29%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SuZhou-Joe SuZhou-Joe force-pushed the new-core-plugin-to-dynamic-content-rendering branch from db3ae33 to ad9eb05 Compare July 9, 2024 09:00
SuZhou-Joe added a commit that referenced this pull request Jul 9, 2024
Copy link
Contributor

❌ Empty Changelog Section

The 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.

@ruanyl ruanyl changed the title WIP: New core plugin for dynamic content rendering New core plugin for dynamic content rendering Jul 12, 2024
@ruanyl ruanyl marked this pull request as ready for review July 12, 2024 12:48
Copy link
Contributor

❌ Empty Changelog Section

The 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.

@ruanyl ruanyl force-pushed the new-core-plugin-to-dynamic-content-rendering branch from c79d64d to 95b6336 Compare July 18, 2024 11:01
Copy link
Member

@ashwin-pc ashwin-pc left a 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
  }),
});

src/plugins/home/public/plugin.ts Outdated Show resolved Hide resolved
registerPage: ContentManagementService['registerPage'];
}
export interface ContentManagementPluginStart {
registerContentProvider: (provider: ContentProvider) => void;
Copy link
Member

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?

Copy link
Member Author

@ruanyl ruanyl Jul 19, 2024

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.

Copy link
Member

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/app.tsx Outdated Show resolved Hide resolved
title: section.title ?? 'test',
query: {
query: '',
language: 'lucene',
Copy link
Member

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?

Copy link
Member Author

@ruanyl ruanyl Jul 19, 2024

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

Comment on lines +88 to +90
if (props.section.kind === 'card') {
return <CardSection {...props} />;
}
Copy link
Member

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?

@ruanyl
Copy link
Member Author

ruanyl commented Jul 20, 2024

Checked that the failed cypress in discover_table.spec.js is unrelated to this PR, merging now.

@ruanyl ruanyl merged commit cc6949b into opensearch-project:main Jul 20, 2024
127 of 130 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 20, 2024
* 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>
ruanyl pushed a commit that referenced this pull request Jul 21, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants