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

Fixed metadata editing #493

Merged
merged 13 commits into from
Aug 19, 2024
Merged

Fixed metadata editing #493

merged 13 commits into from
Aug 19, 2024

Conversation

roll
Copy link
Collaborator

@roll roll commented Aug 9, 2024

Copy link

cloudflare-workers-and-pages bot commented Aug 9, 2024

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: a1ef385
Status: ✅  Deploy successful!
Preview URL: https://ce807ca0.opendataeditor.pages.dev
Branch Preview URL: https://467-fix-schema-editing.opendataeditor.pages.dev

View logs

@pdelboca
Copy link
Member

@roll I'm still seeing the error when running this branch.

Check out the steps on how I reproduce the error:

simplescreenrecorder-2024-08-10_13.18.17.webm

@roll
Copy link
Collaborator Author

roll commented Aug 12, 2024

@pdelboca
Do you use the same file that you uploaded before the fix? It might be in a broken state already. Can you please try from scratch?

@pdelboca
Copy link
Member

@roll it happens with all new files. See attached video on how to replicate the error

simplescreenrecorder-2024-08-12_13.46.39.webm

@roll
Copy link
Collaborator Author

roll commented Aug 12, 2024

@pdelboca
Thanks, I can reproduce it. It works as expected unless we delete some text in metadata editor. When it deletes the text it issues delete-multiple-cells table event for some reason (cc @guergana). I'm going to investigate and fix it

@guergana
Copy link
Collaborator

delete-multiple-cells

  // works automatically, doesnt need to be passed to <TableEditor with onKeyPress
  useKeyPress(['delete', 'backspace'], () => {
    store.deleteMultipleCells(cellSelection)
  })

the keypress event seems to be activated outside the table editor 🙈 @roll

@roll
Copy link
Collaborator Author

roll commented Aug 14, 2024

No problem @guergana, I'm quick fixing this just by checking if there is an actual cell selection (later will be even safer after #494)

@roll
Copy link
Collaborator Author

roll commented Aug 16, 2024

@pdelboca
Can you please test it again?

@romicolman
Copy link
Collaborator

Hi all!

I tested the implementation for issue #467 and it works OK on Mac. However, I could not test the change for #490.

@roll
Copy link
Collaborator Author

roll commented Aug 19, 2024

Hi @romicolman,

The hover on errors returned for me after the changes in this PR (although I wasn't trying to fix it deliberately)

Screenshot from 2024-08-19 09-59-39

@roll roll requested a review from romicolman August 19, 2024 12:43
Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

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

It works on Mac!

@roll
Copy link
Collaborator Author

roll commented Aug 19, 2024

Thanks!

@roll roll merged commit 8a410fc into main Aug 19, 2024
9 checks passed
@roll roll deleted the 467/fix-schema-editing branch August 19, 2024 13:09
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.

Error when editing the schema of the resource
4 participants