T/1: Implement Table UI #23

jodator · 2018-05-29T14:44:54Z

Suggested merge commit message (convention)

Feature: Implement the base Table UI.

Additional information

Requires changes in

ui: ListItemView should react to isEnabled state changes. ckeditor5-ui#390
theme lark: T/ckeditor5 ui/389: Update the listitem styles to reflect changes in UI. ckeditor5-theme-lark#171

…table-merge-cell, and table-headers.

…d and SetHeaderRowCommand.

oleq · 2018-05-30T11:31:21Z

Why is the table inserted after current block instead of splitting it (see v4 https://ckeditor.com/ckeditor-4/)? It's not very intuitive IMO.

oleq · 2018-05-30T11:32:46Z

There's somethign wrong with the position of the BalloonToolbar when used for the link in the table because it remains attached to the table, not to the link

Not sure which piece of code is broken, TableUI or something else.

Includes: - Row, Col, Cell icons from https://github.com/ckeditor/ckeditor5-table/issues/1 - Alternate Table icon with 3 x 3 grid to match the row/col/cell icons above - Merge/Unmerge toggle states in case they are wanted instead of cell drop-down once multi-cell selection is working

jodator · 2018-05-30T11:49:25Z

Why is the table inserted after current block instead of splitting it

That's why we need this Table UI fast :D Things as this will come as one can actually test tables with UI better. It's the way how InsertTableCmmand was implemented. As this is not UI related I'll report this as a follow up and fix it in this iteration.

jodator · 2018-05-30T12:28:53Z

Not sure which piece of code is broken, TableUI or something else.

After quick checkup with I there might be two issues:

the table feature will show & update position of the balloon but somehow the link plugin contents are inserted so either race condition of which plugin should show baloon on mouse click
when updating selection with keyboard the link plugin updates position of the balloon with data that looks OK ({target: a.ck-link_selected}) but the balloon stays attached to the table.

So I can see that table toolbar could better check when to show it's toolbar.

ps.: I've found some glitch here:
https://github.com/ckeditor/ckeditor5-ui/blob/9b4d88863f2f817a42dd436a3b185a3849ca124a/src/panel/balloon/contextualballoon.js#L196

The position returned here does not match the last one passed to the updatePosition as in the _stack the returned position is for the table and not for the link. But without further investigation I'm not able to see if it is problem of how table uses toolbar or the link.

oleq · 2018-05-30T12:40:17Z

There's a problem with the color of the arrow too

oleq · 2018-05-30T12:42:25Z

the table feature will show & update position of the balloon but somehow the link plugin contents are inserted so either race condition of which plugin should show baloon on mouse click

It could boil down to ckeditor/ckeditor5#852 and this is bad :/

oleq · 2018-05-30T12:44:30Z

Table toolbar also hijacks the main balloon editor toolbar, which is a serious issue because it makes the feature incompatible with the BalloonEditor creator.

oleq · 2018-05-30T12:47:46Z

BTW where's the drag handler as in https://github.com/ckeditor/ckeditor5-table/issues/1#issuecomment-392034926? Is this also a followup?

jodator · 2018-05-30T12:50:21Z

BTW where's the drag handler as in #1 (comment)? Is this also a followup?

Hmm. Good question. I might assumed this as a widget feature (so also for an image) and as such could have follow-up. If not R- this PR and I'll add this. WDYT?

oleq · 2018-05-30T12:57:10Z

Hmm. Good question. I might assumed this as a widget feature (so also for an image) and as such could have follow-up. If not R- this PR and I'll add this. WDYT?

OK, I created an issue in ckeditor5-widget. By default (until we implement the propoer drag and drop), only the table widget is supposed to have this selection (later: drag) handler. So it must be explicitely enabled by a widget (e.g. as a property).

oleq · 2018-05-30T14:20:43Z

We also need to figure out how to keep the focused cell border on the top

Note: position: relative and z-index don't help.

oleq · 2018-05-30T14:24:54Z

@dkonopka Can you check that out #23 (comment)? It looks like a general issue – in the image toolbar too. Also a regression as I recall we've already fixed that twice or so :P

jodator · 2018-05-30T14:28:37Z

@oleq there's also this issue:

jodator · 2018-05-30T14:38:06Z

@oleq The only thing I can see to help this is:

table {
    border-collapse: separate;
}
td {
     border-style: solid;
}

But then the table borders are 2px wide:

dtwist · 2018-05-30T21:44:38Z

Use outline instead of border:
https://codepen.io/david-twist/pen/NMQOwO?editors=1000

.highlight {
  outline: 3px solid rgb(71, 164, 245);
  outline-offset: -1px; /* progressive enhancement - no IE support */
  box-shadow: inset 2px 2px 4px rgba(0, 0, 0, .2);
}

oleq · 2018-05-30T14:08:35Z

tests/manual/table.js


 ClassicEditor
 	.create( document.querySelector( '#editor' ), {
-		plugins: [ ArticlePluginSet, Table ],
+		plugins: [ ArticlePluginSet, Table, TableToolbar ],


The manual test should get a description/scenario since now we have a UI. ATM it is empty.

oleq · 2018-06-04T11:21:05Z

src/commands/setheadercolumncommand.js

+
+		const { column } = tableUtils.getCellLocation( tableCell );
+
+		const columnsToSet = column + 1 !== currentHeadingColumns ? column + 1 : column;


Please ify it

let { column } = tableUtils.getCellLocation( tableCell ); if ( column !== currentHeadingColumns - 1 ) { column ++; }

oleq · 2018-06-04T11:22:35Z

src/commands/setheadercolumncommand.js

+	}
+
+	/**
+	 * @inheritDoc


I'm pretty sure it does more than that. It must be explained (see: linkcommand.js)

oleq · 2018-06-04T11:22:59Z

src/commands/setheadercolumncommand.js

+	}
+
+	/**
+	 * Checks if table cell is in heading section.


a table cell
in the heading section

oleq · 2018-06-04T11:26:14Z

src/commands/setheadercolumncommand.js

+
+		this.isEnabled = !!tableParent;
+
+		this.value = this.isEnabled && this._isInHeading( position.parent, tableParent );


Why is the value dependent on isEnabled? https://docs.ckeditor.com/ckeditor5/latest/api/module_core_command-Command.html#member-value does not mention that and I couldn't find another command which acts like that.

I'll update this piece of code as it does not make sense to check if selection is in heading when there is no table.

oleq · 2018-06-04T12:21:39Z

src/ui/inserttableview.js

+}
+
+/**
+ * Single grid box view element.


"A single". A reference to the class using the box would also be nice ;-)

oleq · 2018-06-04T12:22:49Z

src/ui/utils.js

+}
+
+/**
+ * Returns the positioning options that control the geometry of the


that controls

oleq · 2018-06-04T12:27:57Z

src/utils.js

+}
+
+/**
+ * Checks if a given view element is an table widget.


oleq · 2018-06-04T12:28:06Z

src/utils.js

+const tableSymbol = Symbol( 'isImage' );
+
+/**
+ * Converts a given {@link module:engine/view/element~Element} to an table widget:


oleq · 2018-06-04T12:28:31Z

src/utils.js

+}
+
+/**
+ * Checks if an table widget is the only selected element.


oleq · 2018-06-04T12:32:19Z

I also see a warning in the manual test

toolbarview-item-unavailable: The requested toolbar item is unavailable. Read more: https://docs.ckeditor.com/ckeditor5/latest/framework/guides/support/error-codes.html#error-toolbarview-item-unavailable
{name: "splitCell"}

jodator · 2018-06-04T15:21:39Z

@oleq All the issues were fixed - the only one that I didn't touch is this one as I wrote there I've wrote this docs basically the same way as ImageToolbar one (hence an table in so many places ;) ).

jodator and others added 19 commits May 29, 2018 12:45

Added: Initial row & column dropdowns.

6fd5103

Added: Initial mergeCell dropdown.

c43ecb3

Tests: Fix TableUI tests.

1026056

Tests: Add missing tests for TableUI.

b21451f

Added: Initial splitCell dropdown.

e45ba7f

Added: Initial TableToolbar implementation.

9dde19b

Added table icons: table, table-row, table-column, table-split-cell, …

b2919df

…table-merge-cell, and table-headers.

Changed: Use proper icons for buttons in UI.

a8d9a5c

Other: Refactored SetTableHeadersCommand to two SetHeaderColumnComman…

3d0ede9

…d and SetHeaderRowCommand.

Changed: Bind dropdown item isActive to header commands.

9f8a1be

Changed: Add insertTable dropdown.

1404f22

Tests: Fix SetHeaderRowCommand.

72c9709

Other: Remove focus handling from InsertTableView.

e797859

Other: Update CSS classes to be more BEM & update color values.

67244ca

Docs: Update InsertTableView docs.

a5a3140

Changed: Move splitCell dropdown items to mergeCell dropdown.

5e6744b

Docs: Update ui/utils module docs.

710b8a6

Docs: Update Table UI plugins docs.

969347b

Other: Add UI components label to translation service.

27e9222

jodator requested a review from oleq May 30, 2018 07:30

oleq added 3 commits May 30, 2018 15:52

Merge branch 'dtwist-t/1' into t/1

dbb0a53

Removed unused icons. Refined row, column, table and merge icons.

df93871

Tests: Small adjustments to table styles in the manual test.

8d95244

oleq suggested changes Jun 4, 2018

View reviewed changes

jodator added 5 commits June 4, 2018 14:48

Code style: Fixed some minor code style issues.

0608377

Tests: Update table manual test with description and minor fixes.

4d7e2c7

Docs: Update table documentation.

a50de97

Docs: Remove invalid link.

7e03fe5

Docs: fixed typo.

c887db8

Merge branch 'master' into t/1

cf7f363

oleq merged commit b8b9799 into master Jun 5, 2018

oleq deleted the t/1 branch June 5, 2018 13:33

This was referenced Oct 9, 2019

Toolbar positioning and collision issues ckeditor/ckeditor5#3180

Closed

Disappearing cell selection border ckeditor/ckeditor5#3183

Closed

The tip of the balloon holding a toolbar has a different color ckeditor/ckeditor5#3411

Closed

jodator mentioned this pull request Oct 9, 2019

The ImageToolbar class might extracted to ui module as a general class or helper. ckeditor/ckeditor5#5461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/1: Implement Table UI #23

T/1: Implement Table UI #23

jodator commented May 29, 2018 •

edited

Loading

oleq commented May 30, 2018

oleq commented May 30, 2018

jodator commented May 30, 2018 •

edited

Loading

jodator commented May 30, 2018

oleq commented May 30, 2018

oleq commented May 30, 2018

oleq commented May 30, 2018 •

edited

Loading

oleq commented May 30, 2018

jodator commented May 30, 2018

oleq commented May 30, 2018

oleq commented May 30, 2018

oleq commented May 30, 2018

jodator commented May 30, 2018

jodator commented May 30, 2018

dtwist commented May 30, 2018 •

edited

Loading

oleq May 30, 2018

oleq Jun 4, 2018 •

edited

Loading

oleq Jun 4, 2018

oleq Jun 4, 2018

oleq Jun 4, 2018

jodator Jun 4, 2018

oleq Jun 4, 2018

oleq Jun 4, 2018

oleq Jun 4, 2018

oleq Jun 4, 2018

oleq Jun 4, 2018

oleq commented Jun 4, 2018 •

edited

Loading

jodator commented Jun 4, 2018


		const { column } = tableUtils.getCellLocation( tableCell );

		const columnsToSet = column + 1 !== currentHeadingColumns ? column + 1 : column;


		this.isEnabled = !!tableParent;

		this.value = this.isEnabled && this._isInHeading( position.parent, tableParent );

T/1: Implement Table UI #23

T/1: Implement Table UI #23

Conversation

jodator commented May 29, 2018 • edited Loading

Suggested merge commit message (convention)

Additional information

oleq commented May 30, 2018

oleq commented May 30, 2018

jodator commented May 30, 2018 • edited Loading

jodator commented May 30, 2018

oleq commented May 30, 2018

oleq commented May 30, 2018

oleq commented May 30, 2018 • edited Loading

oleq commented May 30, 2018

jodator commented May 30, 2018

oleq commented May 30, 2018

oleq commented May 30, 2018

oleq commented May 30, 2018

jodator commented May 30, 2018

jodator commented May 30, 2018

dtwist commented May 30, 2018 • edited Loading

Choose a reason for hiding this comment

oleq Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleq commented Jun 4, 2018 • edited Loading

jodator commented Jun 4, 2018

jodator commented May 29, 2018 •

edited

Loading

jodator commented May 30, 2018 •

edited

Loading

oleq commented May 30, 2018 •

edited

Loading

dtwist commented May 30, 2018 •

edited

Loading

oleq Jun 4, 2018 •

edited

Loading

oleq commented Jun 4, 2018 •

edited

Loading