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

fix: paste multiple line to table issue #3906

Merged
merged 2 commits into from Jun 7, 2023

Conversation

luka-nextcloud
Copy link
Contributor

@luka-nextcloud luka-nextcloud commented Mar 7, 2023

πŸ“ Summary

πŸ–ΌοΈ Screenshots

🏚️ Before
image

🏑 After
image

🚧 TODO

  • ...

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@luka-nextcloud luka-nextcloud self-assigned this Mar 7, 2023
@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch from 2a54b60 to ec14f8b Compare March 7, 2023 07:08
@cypress
Copy link

cypress bot commented Mar 7, 2023

Passing run #10056 β†—οΈŽ

0 143 1 0 Flakiness 0

Details:

fix: paste multiple line to table issue
Project: Text Commit: 35320faec9
Status: Passed Duration: 03:20 πŸ’‘
Started: Jun 7, 2023 9:18 PM Ended: Jun 7, 2023 9:22 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this @luka-nextcloud! I have some comments 😊

src/nodes/Table/Table.js Outdated Show resolved Hide resolved
src/nodes/Table/Table.js Outdated Show resolved Hide resolved
@mejo-
Copy link
Member

mejo- commented Mar 7, 2023

@luka-nextcloud also, how about adding a Cypress test for this?

@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch from ec14f8b to 39df7ed Compare March 14, 2023 19:45
@juliushaertl juliushaertl added this to the Nextcloud 27 milestone Mar 21, 2023
@juliushaertl juliushaertl added bug Something isn't working 2. developing labels Mar 24, 2023
@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch from 023ccec to 343d060 Compare March 28, 2023 13:25
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Thanks, works well now @luka-nextcloud. I have some minor remarks, mainly to make the code more readable and to fix the failing linting tests. Please also squash all commits into one. Afterwards the PR is good to be merged from my side.

I'm not sure whether deconstructing the slice children and reassembling them into a new fragment is really necesary, but I didn't find a simpler solution myself.

src/nodes/Table/Table.js Outdated Show resolved Hide resolved
src/nodes/Table/TableCell.js Outdated Show resolved Hide resolved
src/nodes/Table/TableCell.js Show resolved Hide resolved
src/nodes/Table/TableCell.js Show resolved Hide resolved
@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch 2 times, most recently from 13b3fa2 to 97bf10c Compare March 29, 2023 15:01
@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch from ce23640 to 140a1aa Compare March 31, 2023 09:55
@juliushaertl juliushaertl self-requested a review April 6, 2023 13:23
@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch from 140a1aa to 4621b08 Compare April 18, 2023 15:38
Copy link

@datenangebot datenangebot left a comment

Choose a reason for hiding this comment

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

Code looks good,

green CI and usage of the tasks (initial issue description) would be appreciated

@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch from 4621b08 to a9c25ef Compare May 8, 2023 08:32
@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch from a9c25ef to b917a02 Compare May 8, 2023 18:33
@juliushaertl
Copy link
Member

Please check the cypress test again as it seems ot be failing after moving it

@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch from b917a02 to 52e9ed8 Compare May 17, 2023 05:37
@juliushaertl
Copy link
Member

/compile

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Please squash the commits as discussed

@luka-nextcloud luka-nextcloud force-pushed the bug/copy-paste-multiple-lines-to-table branch 2 times, most recently from 55020ea to d6b4585 Compare May 24, 2023 17:36
@luka-nextcloud
Copy link
Contributor Author

@mejo- Your comments were already resolved. Could you please approve?

@juliushaertl juliushaertl force-pushed the bug/copy-paste-multiple-lines-to-table branch from d6b4585 to 99185f6 Compare June 6, 2023 07:02
Signed-off-by: Luka Trovic <luka@nextcloud.com>
@juliushaertl juliushaertl force-pushed the bug/copy-paste-multiple-lines-to-table branch from 99185f6 to 4fea236 Compare June 7, 2023 21:00
@juliushaertl
Copy link
Member

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@juliushaertl juliushaertl merged commit 71697ce into main Jun 7, 2023
32 checks passed
@delete-merged-branch delete-merged-branch bot deleted the bug/copy-paste-multiple-lines-to-table branch June 7, 2023 21:39
@mejo-
Copy link
Member

mejo- commented Jun 8, 2023

/backport 4fea236 to stable27

@mejo-
Copy link
Member

mejo- commented Jun 8, 2023

/backport 4fea236 to stable26

@mejo-
Copy link
Member

mejo- commented Jun 8, 2023

/backport 4fea236 to stable25

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin/stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

mejo- added a commit that referenced this pull request Jul 7, 2023
This replaces the broken approach from #3906.

* Only regard paste handler if pasting to a table cell (Fixes: #4443)
* Add all (marked) text nodes with newlines in between
* Only add a newline for non-text nodes to prevent newlines in between
  text with changing marks.

Signed-off-by: Jonas <jonas@freesources.org>
juliushaertl pushed a commit that referenced this pull request Jul 10, 2023
This replaces the broken approach from #3906.

* Only regard paste handler if pasting to a table cell (Fixes: #4443)
* Add all (marked) text nodes with newlines in between
* Only add a newline for non-text nodes to prevent newlines in between
  text with changing marks.

Signed-off-by: Jonas <jonas@freesources.org>
backportbot-nextcloud bot pushed a commit that referenced this pull request Jul 10, 2023
This replaces the broken approach from #3906.

* Only regard paste handler if pasting to a table cell (Fixes: #4443)
* Add all (marked) text nodes with newlines in between
* Only add a newline for non-text nodes to prevent newlines in between
  text with changing marks.

Signed-off-by: Jonas <jonas@freesources.org>
backportbot-nextcloud bot pushed a commit that referenced this pull request Jul 10, 2023
This replaces the broken approach from #3906.

* Only regard paste handler if pasting to a table cell (Fixes: #4443)
* Add all (marked) text nodes with newlines in between
* Only add a newline for non-text nodes to prevent newlines in between
  text with changing marks.

Signed-off-by: Jonas <jonas@freesources.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasting into table destroys it
6 participants