T/2: Initial implementation of Alignment feature #6
Conversation
src/alignmentediting.js
Outdated
| /** | ||
| * @inheritDoc | ||
| */ | ||
| static get requires() { |
src/alignmentcommand.js
Outdated
| * @private | ||
| * @returns {Boolean} Whether the command should be enabled. | ||
| */ | ||
| _isDefault() { |
There was a problem hiding this comment.
It is the same method as in AligmentEditing class. Maybe we could get rid of this duplicated code? We could pass this information in command's constructor or make it an utility function same as isSupported. WDYT?
There was a problem hiding this comment.
I've made this as checking if command is default one will be a bit more complex with RTL support. But until we do not know everything how RTL support will be provided it's better to make this as we cannot predict what arguments isDefault would need.
I'd change that to a private property.
src/alignmentediting.js
Outdated
| afterInit() { | ||
| const schema = this.editor.document.schema; | ||
|
|
||
| // Disallow alignment on fiqcaption |
src/alignmentui.js
Outdated
| const buttonView = new ButtonView( locale ); | ||
|
|
||
| buttonView.set( { | ||
| label: t( upperFirst( style ) ), |
There was a problem hiding this comment.
t() function should be executed with strings directly. See how ImageStyles plugin deals with localization for default styles. I would propose to use "align" word in labels: "Align left", "Align right", "Align center" and "Justify".
src/alignmentediting.js
Outdated
| if ( schema.hasItem( 'caption' ) ) { | ||
| schema.disallow( { name: 'caption', attributes: 'alignment' } ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This disallows alignment attribute on all caption elements (not only those from images). I don't know if this is something wrong just want to point that out. We are assuming here that caption element will always be used in same context as in image feature.
There was a problem hiding this comment.
The only other element that could have (and probably will have) caption is table.
Probably it's name will be problematic as it may be confused with figcaption ;)
Anyway I suspect that Alignment feature would be disallowed there as in <figcaption> so probably such disallow() is OK.
tests/alignmentcommand.js
Outdated
| expect( defaultAlignmentCommand ).to.have.property( 'value', true ); | ||
| } ); | ||
|
|
||
| it( 'is false when selection is not block that has different alignment', () => { |
There was a problem hiding this comment.
"is false when selection is inside block that has different alignment (default style)" WDYT?
tests/alignmentcommand.js
Outdated
| } ); | ||
|
|
||
| describe( 'value', () => { | ||
| it( 'is true when selection is in block with commend type alignment', () => { |
tests/alignmentcommand.js
Outdated
| ); | ||
| } ); | ||
|
|
||
| it( 'removes alignment from all selected blocks even if one has not alignment', () => { |
There was a problem hiding this comment.
"... if one has no alignment defined" ?
tests/alignmentediting.js
Outdated
| expect( doc.schema.check( { name: 'heading1', attributes: 'alignment' } ) ).to.be.true; | ||
| } ); | ||
|
|
||
| it( 'is disallowed on figcaption', () => { |
There was a problem hiding this comment.
This should check if it is disabled on caption elements in model.
tests/manual/alignmentconfig.js
Outdated
| .create( document.querySelector( '#editor' ), { | ||
| plugins: [ ArticlePluginSet, Alignment ], | ||
| toolbar: [ | ||
| 'headings', 'bold', 'italic', 'link', 'alignLeft', 'alignRight', 'alignCenter', 'alignJustify', 'bulletedList', 'numberedList', |
There was a problem hiding this comment.
Adding alignLeft and alignRight buttons cause warrnings in the console as they're not present with this configuration.
|
One bug spotted: when you select all content in |
Suggested merge commit message (convention)
Feature: Initial implementation. Closes ckeditor/ckeditor5#2113.
Additional information
This PR comes with changes in other packages branches:
Merge commit message for related branches:
Feature: Introduce ckeditor5-alignment package.Internal: Remove align icons from theme folder that are moved to ckeditor5-alignment plugin.Misc
styleattribute on view will be replaced if any other feature would also define to attributestyleconverter.