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

CKBox: Update configuration options passed down to CKBox instance by ckbox-plugin #16473

Merged
merged 7 commits into from
Jun 25, 2024

Conversation

illia-stv
Copy link
Contributor

@illia-stv illia-stv commented Jun 4, 2024

Suggested merge commit message (convention)

Feature (ckbox): Added more configuration options passed down to the CKBox. Closes #3695


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@illia-stv illia-stv self-assigned this Jun 4, 2024
packages/ckeditor5-ckbox/src/ckboxconfig.ts Outdated Show resolved Hide resolved
packages/ckeditor5-ckbox/src/ckboxconfig.ts Outdated Show resolved Hide resolved
height: number;
};

export type CKBoxCategories = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why export type if the config uses export interface? Seems like an unnecessary inconsistency.

};
};

export type CKBoxView = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why export type if the config uses export interface? Seems like an unnecessary inconsistency.

hideMaximizeButton: boolean;
};

export type CKBoxUpload = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why export type if the config uses export interface? Seems like an unnecessary inconsistency.

Comment on lines 149 to 153
const dialog = ckboxConfig.dialog!;
const categories = ckboxConfig.categories!;
const view = ckboxConfig.view!;
const upload = ckboxConfig.upload!;

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not-null assertions (!)? Below, there are guards (like dialog && dialog.with) for each of those values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know certainly why, but I afraid ckboxConfig.dialog!.width! is not valid. I tried it out and tests are crushed with that expression.

packages/ckeditor5-ckbox/src/ckboxcommand.ts Outdated Show resolved Hide resolved
packages/ckeditor5-ckbox/src/ckboxcommand.ts Outdated Show resolved Hide resolved
packages/ckeditor5-ckbox/src/ckboxcommand.ts Outdated Show resolved Hide resolved
packages/ckeditor5-ckbox/tests/ckboxcommand.js Outdated Show resolved Hide resolved
@arkflpc
Copy link
Contributor

arkflpc commented Jun 18, 2024

Please change the suggested comment message. Update is too general for changelog. Something like "Added more/several options, ....".

@illia-stv illia-stv requested a review from arkflpc June 25, 2024 09:02
@arkflpc arkflpc merged commit 6c0145a into master Jun 25, 2024
6 checks passed
@arkflpc arkflpc deleted the ck/3695-extend-configuration-in-ckbox branch June 25, 2024 12:26
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.

2 participants