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

#6549: Adding a new row in the table copies structure of the selected row. #6748

Merged
merged 6 commits into from
May 15, 2020

Conversation

niegowski
Copy link
Contributor

Suggested merge commit message (convention)

Other (table): Adding a new row in the table copies the structure of the selected row. Closes #6549.


Additional information

@niegowski niegowski requested a review from jodator May 6, 2020 16:51
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

I need to take longer time to think on this to not over-engineer something here.

I wonder if we need copying structure from any not-related row.

packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Show resolved Hide resolved
niegowski added 4 commits May 7, 2020 11:00
# Conflicts:
#	packages/ckeditor5-table/src/commands/insertrowcommand.js
#	packages/ckeditor5-table/tests/commands/insertrowcommand.js
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

@niegowski Sorry for a longer review on this. I think that it would be better to make some calculations here a bit more straightforward. The way how -1 work here is hard to grasp. It do the job but it might require some time for others to dig into this code later on.

Check on some proposals below but anything less complicated is also welcome :)

packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
packages/ckeditor5-table/src/tableutils.js Outdated Show resolved Hide resolved
@niegowski niegowski requested a review from jodator May 14, 2020 13:42
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

This turned out to be a more complex task than it looked at first. Probably like most table related tasks :D

Hopefully we will remember why this looks like this and new comments will help with that :crossed_fingers:

@jodator jodator merged commit 9f20911 into master May 15, 2020
@jodator jodator deleted the i/6549 branch May 15, 2020 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a new row to table with colspans should repeat the structure of the previous row
2 participants