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

Consistent naming strategy for buttons and plugins #8033

Closed
Reinmar opened this issue Sep 6, 2020 · 17 comments · Fixed by #8394
Closed

Consistent naming strategy for buttons and plugins #8033

Reinmar opened this issue Sep 6, 2020 · 17 comments · Fixed by #8394
Assignees
Labels
bc:major Resolving this issue will introduce a major breaking change. domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented Sep 6, 2020

Buttons

Verb + noun (optional):

  • insertTable,
  • linkImage,
  • selectAll,
  • mergeTableCells,
  • exportWord.

Noun + verb:

  • imageUpload,
  • imageInsert.

Noun (usually, a feature name):

  • mediaEmbed,
  • bold,
  • paragraph,
  • heading,
  • restrictedEditing,
  • tableCellProperties.

Observation and proposal

IMO, the "noun + verb" category is an inconsequence. 

Proposal

⚠️  I'd rename both plugins to verb + noun (but maintain backward compat by having aliases).

Plugins

Feature name:

  • Bold,
  • Italic,
  • MediaEmbed,
  • Heading,
  • Paragraph,
  • Clipboard,
  • Image,
  • SpecialCharacters,
  • Table,
  • Widget.

Feature name + sub-feature:

  • HeadingButtons,
  • ImageResize,
  • ImageUpload,
  • ImageResize,
  • ListStyle,
  • SpecialCharactersArrows, ...
  • TableClipboard,
  • TableMouse,
  • TableProperties,
  • TableCellProperties,
  • WidgetResize.

Exceptions:

  • AutoLink
  • TodoList
  • Input and Delete (as opposed to TypingInput and TypingDelete).

Observations

We're quite consistent here. There are either [FeatureName] plugins or [FeatureName][SubFeature] ones. There are some exceptions, but I'd say reasonable.

More importantly, though, we can see that if the [SubFeature] part is a verb, the button name diverges from the plugin name. A special case here is insertTable vs Table and in some sense insertImage if it was registered by Image as proposed in #8002. I don't see a problem here, though. I don't think we need to have plugin == button names equality.

tl;dr: I don't think we need to change anything here. We should hace [FatureName]  and  [FeatureName][SubFeature] plugins with some room for exceptions where it makes sense.


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@Reinmar Reinmar added type:improvement This issue reports a possible enhancement of an existing feature. domain:dx This issue reports a developer experience problem or possible improvement. squad:dx labels Sep 6, 2020
@Reinmar Reinmar changed the title Consistent naming strategy for buttons Consistent naming strategy for buttons and plugins Sep 6, 2020
@Reinmar
Copy link
Member Author

Reinmar commented Sep 6, 2020

TODO:

  • commands,
  • ADR,
  • section in the naming convention guide.

@jodator
Copy link
Contributor

jodator commented Sep 7, 2020

IIRC - we had imageUpload and then we've added insertImage to have it "consistent". However, it looks odd.

For the UI button in #8002 it could make sense to have image button since it handles image related things.

And for this case table might be also acceptable vs insertTable as other features do not have insert (I can think of 'mediaEmbedd' only right now).

@oleq
Copy link
Member

oleq commented Sep 7, 2020

IMO, the "noun + verb" category is an inconsequence.

OTOH it allows grouping button names by features (feature->action: imageDoThis, imageDoThat) that looks good and could make sense for integrators. 

But given the popularity of verb+noun across the project, I agree it's an inconsequence and this should be fixed. Besides, it also does not work in terms of natural language (insert what vs. what insert).

@jodator
Copy link
Contributor

jodator commented Sep 7, 2020

As discussed on the meeting 👍 for the change in image buttons and to describe the naming convention.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 17, 2020

We should change the names in this iteration because we introduced the new imageInsert button (which is a part of a breaking change) and it'd be good to not change its name soon after.

We do however have to remember to fix those names in the changelog as it currently holds the notion of imageInsert :D

@jodator jodator added this to the iteration 36 milestone Sep 22, 2020
@Reinmar
Copy link
Member Author

Reinmar commented Sep 24, 2020

 I reviewed command names and these are trickier. There are more types of their names. However, again some of the image commands stand out. Namely, I'd fix these names: imageInsert, imageResize, imageUpload, todoListCheck. These are the only 4 commands that have noun+verb structure. I'd rename them to insertImage (matches the new button name), resizeImage, uploadImage (matches the new button name) and checkTodoList.

Feature name (noun-based)

Note: Many of these names could also be moved to the next category as they are both nouns and verbs. I moved there only link and undo as these have their verb-based counterparts: unlink and undo.

  • fontColor,
  • fontBackgroundColor,
  • shiftEnter,
  • enter,
  • blockQuote,
  • bold,
  • paragraph,
  • heading,
  • numberedList,
  • bulletedList,
  • mediaEmbed,
  • codeBlock,
  • alignment,
  • code,
  • underline,
  • strikethrough,
  • superscript,
  • subscript,
  • highlight,
  • fontFamily,
  • fontSize,
  • pageBreak,
  • horizontalLine,
  • todoList,
  • italic,

Feature-related (verb-based)

  • selectAll,
  • undo,
  • redo,
  • forwardDelete,
  • delete,
  • mention,
  • removeFormat,
  • indent,
  • outdent,
  • link,
  • unlink,

Feature + sub-feature (noun)

  • imageTextAlternative,
  • imageStyle,
  • tableBorderColor,
  • tableBorderStyle,
  • tableBorderWidth,
  • tableAlignment,
  • tableWidth,
  • tableHeight,
  • tableBackgroundColor,
  • tableCellBorderStyle,
  • tableCellBorderColor,
  • tableCellBorderWidth,
  • tableCellHorizontalAlignment,
  • tableCellWidth,
  • tableCellHeight,
  • tableCellPadding,
  • tableCellBackgroundColor,
  • tableCellVerticalAlignment,

Verb

  • insertParagraph,
  • insertText (previously: input),
  • indentList,
  • outdentList,
  • insertTable,
  • insertTableRowAbove,
  • insertTableRowBelow,
  • insertTableColumnLeft,
  • insertTableColumnRight,
  • removeTableRow,
  • removeTableColumn,
  • splitTableCellVertically,
  • splitTableCellHorizontally,
  • mergeTableCells,
  • mergeTableCellRight,
  • mergeTableCellLeft,
  • mergeTableCellDown,
  • mergeTableCellUp,
  • setTableColumnHeader,
  • setTableRowHeader,
  • selectTableRow,
  • selectTableColumn,
  • indentCodeBlock,
  • outdentCodeBlock,
  • indentBlock,
  • outdentBlock,

Feature + action (verb)

  • imageInsert,
  • imageResize,
  • imageUpload,
  • todoListCheck,

@Reinmar
Copy link
Member Author

Reinmar commented Sep 24, 2020

My biggest issue with all this is that inside the image package now we'd have:

  • insertImage,
  • uploadImage,
  • imageTextAlternative,
  • imageStyle

But the same is true for tables and it never drew my attention – it felt fine so far. So perhaps I'll get used to the image names too.

@jodator
Copy link
Contributor

jodator commented Sep 24, 2020

I'd rename them to insertImage (matches the new button name), resizeImage, uploadImage (matches the new button name) and checkTodoList.

👍 sounds natural

I'd rename them to insertImage (matches the new button name), resizeImage, uploadImage (matches the new button name) and checkTodoList.

👍 I'll second this, having those in tables seems OK so in the image would be OK also. Naming is tricky as we have both noun/verb features. Having two patterns is ok as bold is just OK and I don't know how we could use verb + noun there.

forwardDelete,

Reading above alongside others seems kinda odd, maybe deleteForward? But we could leave that as-is.

Side note: we do delete on backspace and forwardDelete on delete... at least on PC keyboard. Dunno how that's related to Mac.

insertParagraph,

insertTable.

Those two also stands out a bit. So my assumption on reading was OK - it does insert paragraph as opposed to the paragraph command which will change block to a paragraph.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 24, 2020

Reading above alongside others seems kinda odd, maybe deleteForward? But we could leave that as-is.

delete and forwardDelete are inherited from https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand. However, beforeInput fixes these names a bit so you have delete* and delete*Forward: https://www.w3.org/TR/input-events-2/#interface-InputEvent-Attributes. 

Actually, beforeInput is a good source as it's quite consistent in starting the action name from a verb. The only (funny) exception is historyUndo/Redo :D

Anyway, I'm fine to have deleteForward. This will read better and translate better to beforeInput.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 24, 2020

Doh, one more question/problem – should class names be changed too? In general – should they follow the name under which the button/command is registered or the plugin name + sub-feature convention?

I'd go with following the name of the button/command.

@jodator
Copy link
Contributor

jodator commented Sep 24, 2020

I'd go with following the name of the button/command.

Oh yeah. Definitely 👍.

@Reinmar Reinmar modified the milestones: iteration 36, iteration 37 Sep 28, 2020
@Reinmar Reinmar added the bc:major Resolving this issue will introduce a major breaking change. label Oct 12, 2020
@Reinmar Reinmar modified the milestones: iteration 37, backlog Oct 12, 2020
@jodator jodator added the intro Good first ticket. label Oct 26, 2020
@Reinmar Reinmar modified the milestones: backlog, iteration 38 Oct 26, 2020
@mlewand
Copy link
Contributor

mlewand commented Oct 28, 2020

@Reinmar @jodator I wanted to sum up what needs to be done for @psmyrek. Please check the below and see if I'm missing something.

  • Add verb+noun button names for all the noun+verb, while keeping old names for backward compatibility (it happens to be just 2 image buttons mentioned in "Noun + verb" of the main issue comment).
    • Be sure to update button class name too to follow this change.
  • Rename Feature + action (verb) commands to verb+noun as mentioned in the comment.
    • Be sure to update command class name too to follow this change.
  • Write an ADR.
  • Document this (allowed naming formats) in the naming convention guide.
  • Update changed references in the changelog.

@psmyrek
Copy link
Contributor

psmyrek commented Oct 29, 2020

@Reinmar @jodator What about the imageResize button (which fits in the "noun + verb" category)? Should it be also aligned with related command resizeImage?

@jodator
Copy link
Contributor

jodator commented Oct 29, 2020

Those should be resizeImage 👍 . But please share the summary so we can look at it once again.

@psmyrek
Copy link
Contributor

psmyrek commented Oct 30, 2020

Below is a summary of what I will change:

Buttons

imageInsertinsertImage
imageUploaduploadImage
imageResizeresizeImage

Commands

imageInsertinsertImage
imageUploaduploadImage
imageResizeresizeImage
todoListCheckcheckTodoList
forwardDeletedeleteForward

@Reinmar @jodator Should the commands also have an alias (old name) for backward compatibility, or just buttons?

@psmyrek
Copy link
Contributor

psmyrek commented Oct 30, 2020

Should the file names also be renamed to reflect the class name change (e.g. imageuploadcommand.js to uploadimagecommand.js)?

Currently, files are sorted nicely in the IDE (basing on the "feature" part), but some file names will not match the class name that will be defined in them:

src/
└── imageupload/
    ├── imageuploadcommand.js    ⭠ this file will contain the `UploadImageCommand` class
    ├── imageuploadediting.js
    ├── imageuploadprogress.js
    ├── imageuploadui.js
    └── utils.js

On the other hand, if we change the names of some files, then they will not sort well in the IDE anymore...

@jodator
Copy link
Contributor

jodator commented Oct 30, 2020

 Should the commands also have an alias (old name) for backward compatibility, or just buttons?

Both.

ps.: I've edited your post and added: forwardDeletedeleteForward change

Should the file names also be renamed to reflect the class name change (e.g. imageuploadcommand.js to uploadimagecommand.js)?

Yes. (remember to change also test file name and the describe entries as well.

For the file names vs feature path. I think that this is OK.

src/
└── imageupload/
    ├── uploadimagecommand.js    ⭠ this file will contain the `UploadImageCommand` class

Because the command name follows a specific convention here it will be easier to find UploadImageCommand class for editor.execute( 'uploadImage' ) command (as described in the docs.

This also brought my attention to keep in mind to update jsdocs paths as well when you'll rename a class. The UploadImageCommand will be in:

/**
 * @module image/imageupload/uploadimagecommand
 */

@jodator jodator modified the milestones: iteration 38, iteration 39 Nov 20, 2020
pomek added a commit that referenced this issue Feb 12, 2021
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc:major Resolving this issue will introduce a major breaking change. domain:dx This issue reports a developer experience problem or possible improvement. intro Good first ticket. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants