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

CB-5918 Add new plugin #3081

Open
wants to merge 41 commits into
base: devel
Choose a base branch
from
Open

CB-5918 Add new plugin #3081

wants to merge 41 commits into from

Conversation

SychevAndrey
Copy link
Contributor

No description provided.

@SychevAndrey SychevAndrey self-assigned this Nov 19, 2024
@SychevAndrey SychevAndrey marked this pull request as ready for review November 19, 2024 12:02
Comment on lines 32 to 34
"@types/react": "*",
"typescript": "^5",
"typescript-plugin-css-modules": "*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please same versions as in other packages

Comment on lines 21 to 42
"@cloudbeaver/core-administration": "^0",
"@cloudbeaver/core-authentication": "^0",
"@cloudbeaver/core-blocks": "^0",
"@cloudbeaver/core-connections": "^0",
"@cloudbeaver/core-data-context": "^0",
"@cloudbeaver/core-di": "^0",
"@cloudbeaver/core-dialogs": "^0",
"@cloudbeaver/core-events": "^0",
"@cloudbeaver/core-executor": "^0",
"@cloudbeaver/core-localization": "^0",
"@cloudbeaver/core-resource": "^0",
"@cloudbeaver/core-root": "^0",
"@cloudbeaver/core-routing": "^0",
"@cloudbeaver/core-sdk": "^0",
"@cloudbeaver/core-ui": "^0",
"@cloudbeaver/core-utils": "^0",
"@cloudbeaver/core-version": "^0",
"@cloudbeaver/core-view": "^0",
"@cloudbeaver/plugin-administration": "^0",
"@cloudbeaver/plugin-holidays": "^0",
"@cloudbeaver/plugin-settings-menu": "^0",
"@cloudbeaver/plugin-top-app-bar": "^0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like too many dependencies for "App logo"

webapp/packages/plugin-app-logo/src/index.ts Show resolved Hide resolved
Comment on lines 21 to 27
override register(): void | Promise<void> {
override register() {
this.topNavService.placeholder.add(Logo, 0);
this.administrationTopAppBarService.placeholder.add(Logo, 0);
this.wizardTopAppBarService.placeholder.add(Logo, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like you need to revert this change because Logo was added separately to the public and administration part on purpose

"@cloudbeaver/core-blocks": "^0",
"@cloudbeaver/core-di": "^0",
"@cloudbeaver/core-utils": "^0",
"@cloudbeaver/plugin-administration": "^0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case this plugin depends on administration, you have two options:
Make two plugins:
plugin-holidays
plugin-holidays-administration

or make a single plugin
plugin-holidays-administration
(in that case, it should not affect the public part of the application)

This is because the administration part is optional as is public part, so we need this ability to turn off one of these parts

Comment on lines 29 to 31
this.topNavService.placeholder.add(HolidayButton, 4);
this.administrationTopAppBarService.placeholder.add(HolidayButton, 4);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same situation here, this service depends both on public and administration part, it needs to be split

export interface IHoliday {
startDate: Date;
endDate: Date;
isHoliday: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

holiday.isHoliaday is a bit weird, it may be active or ongoing or something else.

logoSrc: string;
iconSrc: string;
activeIconSrc?: string;
isCelebrating: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm baffled; what is the difference between isHoliday?
It will be better to name it animationActive and actions like startAnimation, stopAnimation
(or maybe another word, but something to make some meaning difference with isHoliday

webapp/packages/plugin-holidays/src/index.ts Show resolved Hide resolved
Copy link
Member

@Wroud Wroud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of new logo plugins, we need PR in private repo as well

webapp/packages/plugin-holidays/package.json Outdated Show resolved Hide resolved
webapp/packages/plugin-holidays/src/Christmas/Christmas.ts Outdated Show resolved Hide resolved
webapp/packages/plugin-holidays/src/Christmas/Christmas.ts Outdated Show resolved Hide resolved
import { Bootstrap, injectable } from '@cloudbeaver/core-di';
import { TopNavService } from '@cloudbeaver/plugin-top-app-bar';

import { Logo } from './Logo.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use importLazyComponent - so main goal here is to initialize "business" logic as fast as possible and then load UI part so every time you import components in services use importLazyComponent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants