Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

I/6113: Base table selection plugin #230

Merged
merged 118 commits into from
Feb 21, 2020
Merged

I/6113: Base table selection plugin #230

merged 118 commits into from
Feb 21, 2020

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jan 29, 2020

Suggested merge commit message (convention)

Feature: Introduce TableSelection plugin. Closes ckeditor/ckeditor5#6113.


Additional information

  • 🎉 a TableSelection has matured since its first commit in August of 2018.
  • most important things here are: TableSelection plugin itself and the introduced MouseSelectionHandler - the architecture of this feature is meant to have keyboard handler in a similar fashion
  • the conversion uses the highlight mechanism known from the widget (fake selection, etc)
  • if something is not working or not implemented - check the epick first if it is not reported - this PR only enables table selection with a mouse - there are no commands that would support this ATM

# Conflicts:
#	src/commands/mergecellcommand.js
#	src/tableui.js
#	tests/converters/downcast.js
# Conflicts:
#	src/commands/mergecellcommand.js
@jodator
Copy link
Contributor Author

jodator commented Feb 18, 2020

@Reinmar so for the bugs:

  1. Fixed on PR
  2. Moved to UI/UX issue - it is a Chrome issue IMO as on FF it works OK

As for the missing features:

  1. somehow fixed on PR
  2. added a comment in the keyboard selection issue
  3. I've created a bug for this at this needs some time to debug implement. There's a starting point though. Table selection does not cooporate with typing ckeditor5#6284

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly docs. Some minor changes.

I'll create a separate review for more because in the past GitHub failed when there were more than 50 suggestions and... I learned my lesson.

src/tableselection/converters.js Outdated Show resolved Hide resolved
src/tableselection/converters.js Outdated Show resolved Hide resolved
theme/tableediting.css Outdated Show resolved Hide resolved
src/commands/removerowcommand.js Outdated Show resolved Hide resolved
src/tableselection.js Outdated Show resolved Hide resolved
src/tableselection/mouseeventsobserver.js Outdated Show resolved Hide resolved
src/tableselection/mouseeventsobserver.js Outdated Show resolved Hide resolved
src/tableselection/mouseeventsobserver.js Outdated Show resolved Hide resolved
src/tableselection/mouseeventsobserver.js Outdated Show resolved Hide resolved
src/tableselection/mouseeventsobserver.js Outdated Show resolved Hide resolved
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly docs. Some minor changes.

I'll create a separate review for more because in the past GitHub failed when there were more than 50 suggestions and... I learned my lesson.

Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor issues.

src/tableselection/mouseselectionhandler.js Outdated Show resolved Hide resolved
src/tableselection/mouseselectionhandler.js Outdated Show resolved Hide resolved
src/tableselection/mouseselectionhandler.js Outdated Show resolved Hide resolved
src/tableselection/mouseselectionhandler.js Outdated Show resolved Hide resolved
src/tableselection/mouseselectionhandler.js Outdated Show resolved Hide resolved
src/tableselection/mouseselectionhandler.js Outdated Show resolved Hide resolved
src/tableselection/mouseselectionhandler.js Outdated Show resolved Hide resolved
src/tableselection/mouseselectionhandler.js Outdated Show resolved Hide resolved
tests/_utils/utils.js Outdated Show resolved Hide resolved
</style>

<div id="editor">
<p>A table to test selection:</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's content is too homogenous to properly test the feature (as follow-ups has shown). We need more cases, for instance:

  • with the content before and after the table,
  • with a solitary table in the content,
  • with a table that has some cells already split/merged,
  • with a table that has links, block quotes and other content in some cells.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have something already for some of the points in the copy table fragment PR. I can cherry pick it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm not sure how to handle all these issues in one manual test. I've added only a complex table (which covers two of those issues).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create 2 editors? 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we have 4 or more cases :P

But what do you think about loading fixtures on a button? One editor (I'd like to have one editor as copy-paste manual tests enhancement would require an editiable arae side by side to the editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleq
Copy link
Member

oleq commented Feb 20, 2020

@jodator jodator requested a review from oleq February 20, 2020 16:41
# Conflicts:
#	src/commands/utils.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants