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

The dropdown's name doesn't match the standards #2123

Closed
Reinmar opened this issue Jan 19, 2018 · 13 comments · Fixed by ckeditor/ckeditor5-alignment#25
Closed

The dropdown's name doesn't match the standards #2123

Reinmar opened this issue Jan 19, 2018 · 13 comments · Fixed by ckeditor/ckeditor5-alignment#25
Assignees
Labels
package:alignment status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 19, 2018

It's called alignmentDropdown while the heading feature exposes a headings component (which is a dropdown).

We need to decide on one naming scheme.

cc @jodator, @oleq

@Reinmar Reinmar self-assigned this Jan 19, 2018
@oleq
Copy link
Member

oleq commented Jan 19, 2018

fooDropdown approach could be better because in the future we could have other UI components exposing the foo feature e.g. fooList, fooDialog. If we go with foos first, we'll bring an inconsistency with no easy way out.

But the real question is different: do we want to have 2 different UI components for the same feature in a single editor instance?

If yes, then there's no other way than to differentiate e.g. boldButton, boldCheckbox, boldMagicBoxThatSings, each one coming from a different package.

If no, then it's obvious that there's only one UI representation of the feature in the editor and that's it. It's just bold.

@jodator
Copy link
Contributor

jodator commented Jan 19, 2018

To shed some light on this. The Alignment & Highlight features comes with dropdown and individual buttons. In alignment you can have:

  • alignLeft, alignRight, alignCenter and alignJustify buttons or
  • alignmentDropdown dropdown

Similar approach I've showcased to @Reinmar with highlight so you can have:

  • highlight:marker, highlight:greenMarker, etc.. depending on config and
  • highlightDropdown.

Right now both buttons and dropdowns are defined by *UI part of both features as the scheme is that I use defined buttons to create a dropdown with those buttons.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 19, 2018

We certainly should consider cases that a single feature has more than one UI component for it. Otherwise, in rare situations when someone created a mighty build of CKEditor 5 (e.g. with various alternatives of the heading feature), some of the components won't work.

I could agree that:

  • if a component is a button, then just call it foo,
  • if it is a dropdown, then fooDropdown (even if it doesn't also register foo).

In the latter case, the "Dropdown" suffix will work kinda like a plural form (so, instead of headings there will be headingDropdown). This makes sense for two reasons:

  • I had a problem with the headings component, because the feature is called heading and one may not notice the difference.
  • Some words may not work well with a plural form – e.g. alignments sounds worse than alignmentDropdown.

However, what about other type of feature components? Highlight uses a split button – can be it called a "dropdown"? Won't this be confusing?


highlight:marker, highlight:greenMarker, etc.. depending on config and

I don't like the fact that there's alignLeft and highlight:greenMarker. It kinda makes sense, because "align left" works as a sentence, while "highlight greenMarker" not really. But those things are then super unclear to others. So, either we use : when building a command/button name, or we use upperFirst( value ).

And I'm closer to upper-first. Why? Because there are cases like heading1. Or should it be heading:1? Is it a param or is it a whole name of this command? Is alignLeft a whole name? Or should it be align:left? Weird things happen. See e.g. the list of CKEditor 4's config options: https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_config.html. You need a while to understand why there's an underscore in some of these properties.

@jodator
Copy link
Contributor

jodator commented Feb 28, 2018

I'm still leaning towards: feature:option naming (even for alignment) as I'm used to : in various places (eg. events' names). To make things similar I'd drop the Dropdwon suffix.

Solution 1: event separator notation

  • heading
  • alignment
  • alignment:left
  • highlight
  • highlight:greenMarker

The default UI component have its name after a feature. So Feature will have a feature toolbar component (either a button or a dropdown as for now). If a feature introduces a secondary UI component it should be prefixed as feature:name. Where the name part should be directly taken from options/configuration.

For me, the main pro for this is that the dev will not have to think whether heading feature provides dropdown or button or something else. It is straightforward that Heading feature provides heading UI component.
The same goes for others. The Alignment feature provides alignment UI component for the toolbar. After reading docs the dev might discover that Alignment feature provides also a secondary UI components: alignment:left, alignment:right, ... etc.

Solution 2: camel case

  • headingDropdown
  • alignmentDropdown
  • alignLeft/alignmentLeft
  • highlightDropdwon / highlightSplitButton
  • highlightGreenMarker

Requires reading docs to build toolbar. (I know, I know - one would see on examples that heading is a dropdown then the configuration is headingDropdown not heading as other features would suggest).

@jodator
Copy link
Contributor

jodator commented Feb 28, 2018

ps: I'm for option 1 still.

@oleq
Copy link
Member

oleq commented Mar 1, 2018

If a feature introduces a secondary UI component it should be prefixed as feature:name.

That's a good point actually.

Still, what should we do about https://github.com/ckeditor/ckeditor5-heading/issues/74?

  • heading
  • heading:1 – ??? – e.g. we use the ck-heading_heading1 CSS class ATM. Maybe we should provide some user-readable names for headings or some prefix, e.g. heading:size1
  • alignment
  • alignment:left
  • highlight
  • highlight:greenMarker

@Reinmar
Copy link
Member Author

Reinmar commented Mar 1, 2018

I'm quite ok with the featureName:optionName notation, but then we need to figure out what to do with the heading feature and its:

editor.config.define( 'heading', {
	options: [
		{ model: 'paragraph', title: 'Paragraph', class: 'ck-heading_paragraph' },
		{ model: 'heading1', view: 'h2', title: 'Heading 1', class: 'ck-heading_heading1' },
		{ model: 'heading2', view: 'h3', title: 'Heading 2', class: 'ck-heading_heading2' },
		{ model: 'heading3', view: 'h4', title: 'Heading 3', class: 'ck-heading_heading3' }
	]
} );

The model element names are defined here and cannot contain a : character in them. So, the component name would be something like heading:heading1 or we'd have an odd exception and have heading:1 or heading1. Plus, as Olek mentioned, there's CSS class name to be synced as well.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 1, 2018

BTW, there's even bigger problem with command names. They should somehow match component names or there needs to be a simple rule for naming a component which uses a certain command with a certain value.

One idea that we should investigate is whether it's good that we've split the heading command into separate commands. Right now, it's inconsistent with the other comnmands (e.g. alignment).

@jodator
Copy link
Contributor

jodator commented Mar 2, 2018

As @oleq suggested below is a some pseudo code after proposing changes in heading feature.

Adding a button for heading:

editor.ui.componentFactory.add( 'heading:heading1', locale => {
	const button = new ButtonView( locale );
	button.set( { icon, label } );

	const command = editor.commands.get( 'heading' );

	button.bind( 'isOn' ).to( command, 'value', value => value === 'heading1' );
        button.bind( 'isEnabled' ).to( command );

	return button;
} );

Required changes in heading:

  • rename command to: 'heading'
  • make heading command accept value (taken from config.model)
  • heading command value is set to current block name
  • heading command isEnabled is set when any of heading can be inserted (to discuss)
  • to change heading: editor.execute( 'heading', { value: 'heading1' } );

/cc @Reinmar, @oleq Also would like to see input from guys that created other UIs with heading command (@oskarwrobel, @pjasiun ?)

  • to remove heading (make paragraph): command.execute( )
  • rename heading dropdown ui component to heading

@jodator
Copy link
Contributor

jodator commented Mar 2, 2018

OK so to sum up F2F talks, we agreed that component naming should be unified as proposed above.

A general rule is to name default/primary component after a feature:

  • heading (not headings)
  • alignment (not alignmentDropdown)
  • fontSize

If a feature provides secondary component it should be namespaced by a feature name and : so

  • Highlight feature provides: highlight:greenMarker for { model: 'greenMarker' } option.
  • Heading could provide in future heading:heading1 button for { model: 'heading1' } option.
  • Alignment will provide alignment:left (not alignLeft).

Such way of defining component will be easier to construct in code (no upperFirst()) and to remember.

As this is a general rule it should be followed up to the point that other naming for a component makes more sense. But of course this should be avoided and only provide when necessary.
Probably a case:

  • uploadImage provided by a ImageUploadUI
  • Undo provides undo and redo components (not undo:undo/undo:redo).

Also we found that:

  • alignment feature should expose one command that accepts alignment as a value as in highlight, font size, etc. so it will be editor.execute( 'alignment', { value: 'left' } ) not editor.execute( 'alignLeft' ).
  • heading feature should expose one command (as the elders says that was in the old times) that accepts value. (also behavior described here: https://github.com/ckeditor/ckeditor5-alignment/issues/16#issuecomment-369879674).

ps.: Side questions:

  1. Lists: (there's no primary one but I can image a list dropdown)
    a. list:numbered, list:bulleted or
    b. numberedList, bulletedList
  2. ImageUpload should we rename it's component?

@Reinmar
Copy link
Member Author

Reinmar commented Mar 2, 2018

ImageUpload should we rename its component?

Doh... it's called after the uploadImage command. Do we want to rename both of them? editor.execute( 'imageUpload' ) is certainly less cool than editor.execute( 'uploadImage' ), but I think that both are fine. You can both tell that you execute an action called "image upload" and that you upload an image.

BTW, another such example is imageStyleLeft, imageStyleRight. Here we should also change the naming. And again, a call to action like styleImage:<value> sounds the most natural for a command, but even so far we had the imageStyle<Value> version and no one complained so imageStyle:<value> will be fine.

PS. editor.execute( 'alignment', { value: 'left' } ) is yet another example that a command is not a call to action but a code name. It's not the nicest and most beautiful choice, but it's clear enough and reads well enough: "execute the alignment command".

@Reinmar
Copy link
Member Author

Reinmar commented Mar 2, 2018

As this is a general rule it should be followed up to the point that other naming for a component makes more sense.

Totally agree. undo, redo makes sense.

numberedList, bulletedList

Again... Doh. I guess we won't avoid exceptions. Here we could have one command with a param, but I'm completely against that. It's a huge and significant command, not something to be controlled by editor configuration. So, maybe the rules should depend on whether a thing is controlled through the editor configuration. If it is, then one command + values. If isn't, then up to the developer.

@jodator jodator self-assigned this Mar 5, 2018
@jodator
Copy link
Contributor

jodator commented Mar 5, 2018

@Reinmar & @oleq to be 100% sure as I get confused by comments:

BTW, another such example is imageStyleLeft, imageStyleRight. Here we should also change the naming. And again, a call to action like styleImage: sounds the most natural for a command, but even so far we had the imageStyle version and no one complained so imageStyle: will be fine.

numberedList, bulletedList

Do we want commands to be renamed also? I'm guessing not - but I'd rather ask then undo something.

I'm asking about UI component names here: should it be list:bulleted or should be left as it is now: bulletedList.

Also I didn't create alignment:left commands - only an UI components (https://github.com/ckeditor/ckeditor5-alignment/blob/c6ae36010ca8109b3a6000030d0a311c1f53ba18/docs/features/text-alignment.md#common-api).
So there is:

  • command: alignment that accepts value
  • UI buttons: alignment:left, alignment:right, alignment:center and alignment:justify
  • UI dropdwon: alignment

Also I'm going to implement this as a next step (headings -> heading UI component is alredy renamed):

Required changes in heading:

  • rename command to: 'heading'
  • make heading command accept value (taken from config.model)
  • heading command value is set to current block name
  • heading command isEnabled is set when any of heading can be inserted (to discuss)
  • to change heading: editor.execute( 'heading', { value: 'heading1' } );

Reinmar referenced this issue in ckeditor/ckeditor5-alignment Mar 11, 2018
Internal: Update UI components & commands naming standard. Closes #16.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-alignment Oct 8, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:alignment labels Oct 8, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:alignment status:discussion 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.

4 participants