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.

Sign up for GitHub

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

Jump to bottom

Enable multi-cell selection for deleting content of cells #390

Merged
merged 13 commits into from
May 27, 2024
Merged

Enable multi-cell selection for deleting content of cells #390

merged 13 commits into from
May 27, 2024

Conversation

guergana
Copy link
Collaborator

@guergana guergana commented May 14, 2024 •
edited
Loading

@guergana guergana marked this pull request as draft May 14, 2024 16:28
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented May 14, 2024 •
edited
Loading

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 21e2579
Status: ✅  Deploy successful!
Preview URL: https://a4205b81.opendataeditor.pages.dev
Branch Preview URL: https://350-select-multiple-cells.opendataeditor.pages.dev

View logs

@guergana guergana force-pushed the 350-select-multiple-cells branch from d3ec006 to e33692b Compare May 14, 2024 21:58
@guergana guergana force-pushed the 350-select-multiple-cells branch from e33692b to 107f61f Compare May 14, 2024 21:59
@guergana guergana requested review from roll and pdelboca May 17, 2024 11:31
@guergana guergana force-pushed the 350-select-multiple-cells branch from c72cfbd to 9601605 Compare May 17, 2024 16:36
@roll
Copy link
Collaborator

roll commented May 22, 2024 •
edited
Loading

@guergana
I think I found the problem. Take a look at saveEditing action. Note, that we never change the grid values directly. What we do is:

  • add a change to the history
  • initiate grid reloading
    • grid calls loader
    • loader gets data from the server
    • loader applies changes from the history
    • grid gets a new data that is patched

The same flow needs to be used in deleteMultipleCells

@guergana guergana changed the title [WiP] Enable multi-cell selection Enable multi-cell selection May 22, 2024
@guergana guergana requested a review from roll May 22, 2024 17:11
@guergana guergana marked this pull request as ready for review May 22, 2024 17:11
@guergana guergana changed the title Enable multi-cell selection Enable multi-cell selection for deleting content of cells May 24, 2024
@guergana
Copy link
Collaborator Author

@guergana I think I found the problem. Take a look at saveEditing action. Note, that we never change the grid values directly. What we do is:

* add a change to the history

* initiate grid reloading
  
  * grid calls loader
  * loader gets data from the server
  * loader applies changes from the history
  * grid gets a new data that is **patched**

The same flow needs to be used in deleteMultipleCells

Done, thanks!

roll
roll approved these changes May 27, 2024
View reviewed changes
Copy link
Collaborator

@roll roll left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great! It works for me 👍

@guergana guergana merged commit 0de6230 into main May 27, 2024
8 checks passed
@guergana guergana deleted the 350-select-multiple-cells branch May 27, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roll roll roll approved these changes

@pdelboca pdelboca Awaiting requested review from pdelboca

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot delete content of multiple cells
2 participants
@guergana @roll