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

Unified button and command naming convention #8394

Merged
merged 14 commits into from
Feb 12, 2021
Merged

Unified button and command naming convention #8394

merged 14 commits into from
Feb 12, 2021

Conversation

psmyrek
Copy link
Contributor

@psmyrek psmyrek commented Nov 2, 2020

Suggested merge commit message (convention)

Other: Unified buttons, and commands naming convention. Old values are available as aliases. Read more about those changes in the Code style guide. Closes #8033.

Changes in toolbar buttons (before → after):

  • imageUploaduploadImage
  • imageResizeresizeImage
  • imageInsertinsertImage
  • imageResize:*resizeImage:*

Changes in command names:

  • imageInsertinsertImage
  • imageUploaduploadImage
  • imageResizeresizeImage
  • forwardDeletedeleteForward
  • todoListCheckcheckTodoList

MAJOR BREAKING CHANGE (list): The following module list/todolistcheckedcommand~TodoListCheckCommand has been moved to list/checktodolistcommand~CheckTodoListCommand.

MAJOR BREAKING CHANGE (image): The following modules have been moved (before → after)

  • image/image/imageinsertcommand~ImageInsertCommandimage/image/insertimagecommand~InsertImageCommand
  • image/imageresize/imageresizecommand~ImageResizeCommandimage/imageresize/resizeimagecommand~ResizeImageCommand
  • image/imageupload/imageuploadcommand~ImageUploadCommandimage/imageupload/uploadimagecommand~UploadImageCommand

Additional information

ADR has been created: https://github.com/cksource/ckeditor5-internal/pull/426

@jodator jodator self-assigned this Nov 16, 2020
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

Let's call it half-time review but for now I think that you could:

  1. Fix the changes in the CHANGELOG.md files.
  2. Update the docs (see comment).

After @Reinmar yay/no we should finish this PR if changes are needed.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/_snippets/examples/inline-editor.js Show resolved Hide resolved
docs/framework/guides/contributing/code-style.md Outdated Show resolved Hide resolved
packages/ckeditor5-ckfinder/CHANGELOG.md Outdated Show resolved Hide resolved
@jodator
Copy link
Contributor

jodator commented Nov 19, 2020

Additionally, you can also rewrite the merge message as for now it is too mangled (all the changes in one line). If you don't have an idea about this we can think about that later, after all the changes. For start, I would split the massage for each plugin:

Other (image): Renamed UI buttons: imageInsert -> insertImage, imageUpload -> uploadImage, imageResize -> resizeImage. See #8033.
 
Other (list): Renamed todo list command naem todoListCheck -> checkTodoList. See #8033.

etc.

Also, we need to list all the breaking changes for the renamed classes, for instance:

MINOR BREAKING CHANGE (image): The `ImageInsert` command was renamed to `InsertImage`. See #8033.

@psmyrek
Copy link
Contributor Author

psmyrek commented Nov 19, 2020

After this half-time review all requested changes have been done:

  • all CHANGELOG.md files have been reverted,
  • list style in docs/framework/guides/contributing/code-style.md has been fixed,
  • the merge message have been improved,
  • all conflicts are resolved.

@Reinmar
Copy link
Member

Reinmar commented Nov 20, 2020

Oh, we haven't thought about that.

This will be another, big breaking change IMO. However, it might make sense to have this unified after all.

cc @Reinmar -> quick question - are we OK with unifying the configuration options as well?

Oh dear :D Yup, I didn't think about this.

So, I'm for changing it so it's consistent. If we touch anything, let's touch everything.

However, I'm leaning towards postponing this change til January's release. I have a feeling that we'll have a lot of breaking changes in it, or at least significant conceptual changes, so the rename would be less of a PITA there.

@pomek
Copy link
Member

pomek commented Feb 12, 2021

Since many buttons and commands are available as aliases, we don't introduce many minor/major breaking changes. Hence, I'd like to propose a little bit different merge massage:

Other: Unified buttons, and commands naming convention. Old values are available as aliases. Read more about those changes in the [Code style](TODO:URL_TO_OUR_DOCS) guide. Closes #8033.

Changes in toolbar buttons (before ⇒ after):

- `imageUpload` → `uploadImage`
- `imageResize` → `resizeImage`
- `imageInsert` → `insertImage`
- `imageResize:*` → `resizeImage:*`

Changes in command names:

- `imageInsert` → `insertImage`
- `imageUpload` → `uploadImage`
- `imageResize` → `resizeImage`
- `forwardDelete` → `deleteForward`
- `todoListCheck` → `checkTodoList`

MAJOR BREAKING CHANGE (list): The following module `list/todolistcheckedcommand~TodoListCheckCommand` has been moved to `list/checktodolistcommand~CheckTodoListCommand`.

MAJOR BREAKING CHANGE (image): The following modules have been moved (before → after) 

- `image/image/imageinsertcommand~ImageInsertCommand` → `image/image/insertimagecommand~InsertImageCommand`
- `image/imageresize/imageresizecommand~ImageResizeCommand` → `image/imageresize/resizeimagecommand~ResizeImageCommand`
- `image/imageupload/imageuploadcommand~ImageUploadCommand` → `image/imageupload/uploadimagecommand~UploadImageCommand`

@psmyrek, WDYT?

@psmyrek
Copy link
Contributor Author

psmyrek commented Feb 12, 2021

In toolbar buttons there is also imageInsertinsertImage change.

There is no such thing like todoListCheck button or checkTodoList button in CKE5.

In commands there is also imageResizeresizeImage change.

Renamed files:

packages/ckeditor5-image/src/image/imageinsertcommand.js   →   packages/ckeditor5-image/src/image/insertimagecommand.js
packages/ckeditor5-image/src/imageresize/imageresizecommand.js   →   packages/ckeditor5-image/src/imageresize/resizeimagecommand.js
packages/ckeditor5-image/src/imageupload/imageuploadcommand.js   →   packages/ckeditor5-image/src/imageupload/uploadimagecommand.js
packages/ckeditor5-image/tests/image/imageinsertcommand.js   →   packages/ckeditor5-image/tests/image/insertimagecommand.js
packages/ckeditor5-image/tests/imageresize/imageresizecommand.js   →   packages/ckeditor5-image/tests/imageresize/resizeimagecommand.js
packages/ckeditor5-image/tests/imageupload/imageuploadcommand.js   →   packages/ckeditor5-image/tests/imageupload/uploadimagecommand.js
packages/ckeditor5-list/src/todolistcheckcommand.js   →   packages/ckeditor5-list/src/checktodolistcommand.js
packages/ckeditor5-list/tests/todolistcheckcommand.js   →   packages/ckeditor5-list/tests/checktodolistcommand.js

@pomek pomek merged commit e0fcb17 into master Feb 12, 2021
@pomek pomek deleted the i/8033 branch February 12, 2021 12:00
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.

Consistent naming strategy for buttons and plugins
4 participants