Fix for remaining fake selection in advance tab of table properties #599
Conversation
plugins/tableselection/plugin.js
Outdated
| @@ -9,6 +9,7 @@ | |||
| var fakeSelectedClass = 'cke_table-faked-selection', | |||
| fakeSelectedTableClass = fakeSelectedClass + '-table', | |||
| fakeSelectedEditorClass = fakeSelectedClass + '-editor', | |||
| fakeSelectedTableDatasetClass = 'cke-table-faked-selection-table', | |||
There was a problem hiding this comment.
It is more like fakeSelectedTableDataAttribute
plugins/tableselection/plugin.js
Outdated
| @@ -251,7 +252,7 @@ | |||
|
|
|||
| if ( cells.length > 0 ) { | |||
| editor.editable().addClass( fakeSelectedEditorClass ); | |||
| cells[ 0 ].getAscendant( 'table' ).addClass( fakeSelectedTableClass ); | |||
| cells[ 0 ].getAscendant( 'table' ).data( fakeSelectedTableDatasetClass, fakeSelectedTableClass ); | |||
There was a problem hiding this comment.
Maybe you could just use data( fakeSelectedTableDatasetClass, true );?
While the data attribute is removed in line selectedCells.getItem( 0 ).getAscendant( 'table' ).data( fakeSelectedTableDatasetClass, false ); so it can be only present when cells are selected. In this case shorter value could be used (and then you could also simplify CSS selector to check only if attribute is present).
There was a problem hiding this comment.
I used empty string. Setting true just parse it to string, so it reflects with result of attribute: data-cke-table-faked-selection-table="true".
Unfortunately method data is combination of setter and getter, so I cannot omit 2nd attribute when I want to use it as a setter.
Current approach seems to be most convenient. Another option would be design some simple general purpose data attribute, like data-cke-class and assign class values, but it might be confusing.
There was a problem hiding this comment.
Makes sense 👍 .
It is similar behaviour to how Boolean attributes works (e.g. input[checked]). Here we also have only two values, selected / not selected so it is a proper approach IMHO.
|
|
||
| bender.editor = true; | ||
|
|
||
| var test = { |
There was a problem hiding this comment.
Just use bender.test( { ... } ).
| bender.editor = true; | ||
|
|
||
| var test = { | ||
| 'test advance table dialog for ignorig selection class': function() { |
There was a problem hiding this comment.
Please put issue reference link here.
|
|
||
| var bot = this.editorBot; | ||
|
|
||
| // add table with fake selection. |
There was a problem hiding this comment.
Start sentence with an uppercase.
| 1. Right click and select `Table Properties` | ||
| 1. Make sure that selection is now fake (selection colour change to grey) | ||
| 1. Go to `Avanced` tab | ||
| 1. Check `Stylesheet Classes`, it should be empty |
There was a problem hiding this comment.
This should be Expected result, not a part of repro steps.
| @bender-ui: collapsed | ||
| @bender-ckeditor-plugins: dialogadvtab,table,tableselection,wysiwygarea | ||
|
|
||
| 1. Select at least to 2 cells in table |
There was a problem hiding this comment.
Select at least
to2 cells in table.
| @bender-ckeditor-plugins: dialogadvtab,table,tableselection,wysiwygarea | ||
|
|
||
| 1. Select at least to 2 cells in table | ||
| 1. Right click and select `Table Properties` |
There was a problem hiding this comment.
Always end sentences with a ..
|
|
||
| 1. Select at least to 2 cells in table | ||
| 1. Right click and select `Table Properties` | ||
| 1. Make sure that selection is now fake (selection colour change to grey) |
There was a problem hiding this comment.
(selection colour changed to grey)
| @@ -0,0 +1,11 @@ | |||
| @bender-tags: 4.7.2, tc, bug, 579 | |||
|
|
||
| bot.dialog( 'tableProperties', function( dialog ) { | ||
| assert.areSame( '', dialog.getValueOf( 'advanced', 'advCSSClasses' ) ); | ||
| dialog.hide(); |
There was a problem hiding this comment.
There is no need to hide dialog here, especially that it happens after assert so does not have any effect on the test result. This line can be simply removed.
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
yes
This PR contains
What changes did you make?
Move classes to dataset attributes
close #579