accordproject / web-components

React Components for Accord Project
Apache License 2.0
117 stars 94 forks source link

feat(ui-markdown-editor): add support for tables-#39 #379

Closed TC5022 closed 2 years ago

TC5022 commented 2 years ago

Signed-off-by: TC5022 tanyac5022002@gmail.com

Closes #39

Extended the current markdown-editor UI to support, render and edit tables.

Changes

Screenshots or Video

https://user-images.githubusercontent.com/75262300/153654435-1665a686-84b7-4adf-a102-475906654d62.mp4

Author Checklist

TC5022 commented 2 years ago

Requesting a review @dselman @irmerk @mttrbrts

irmerk commented 2 years ago

Very cool so far! I'm curious what happens when we round trip or copy paste?

Round trip: What happens when you insert a table into the markdown initialized in the Storybook? Does it auto render?

Copy paste: What happens when you cut the whole text and paste again, does the table render?

TC5022 commented 2 years ago

The table does not render for roundtrip. But, this issue needs to be handled in the markdown/transform repository. We just need to add support for the table_open block type in this file.

And I'm afraid the copy-paste functionality does not work either. This is because I have just added the basic support to render and edit tables as of now. So this could serve as a new issue as well.

TC5022 commented 2 years ago

Are there any changes to be made ? @irmerk @mttrbrts @dselman @Michael-Grover

TC5022 commented 2 years ago

Thank you for your review @DianaLease. Working on implementing these changes

jeromesimeon commented 2 years ago

@TC5022 @irmerk @DianaLease This is so cool! I'm wondering how I missed this PR. Any chance we could get a demo / update on this at one of our Tech WG calls?

It would be interesting to try and generate the underlying markdown using extensions proposed here: https://github.com/accordproject/markdown-transform/tree/ciceromark-0.4

TC5022 commented 2 years ago

Thank you @jeromesimeon! I would be happy to join the call and present a demo. I am very keen to further work on this feature and make it better by extending the markdown-transform repository to support tables.

TC5022 commented 2 years ago

This looks awesome! A few notes:

  1. We should disallow users from entering a negative number in the number of rows/columns form. Right now, doing so causes an error.
  2. When opening the pop-up when a user clicks inside an existing table, we should probably remove the "number of rows"/"number of columns" input fields since changing these does not change the existing table.

@DianaLease I have updated the code with these changes. Kindly check :)

TC5022 commented 2 years ago

Thanks a lot for your supportive words @dselman!! And yes, I would definitely like to further work on this feature once this PR is merged. As for the delete functionality, you are right as right now only the last row or column gets removed. This is also the case for the addition of rows and columns. I implemented this after looking into such similar web editors. However, I think I can improve that if I get a little guidance. :)

TC5022 commented 2 years ago

@DianaLease any updates on the changes I implemented?

DianaLease commented 2 years ago

@DianaLease any updates on the changes I implemented?

Sorry for the delay. The changes look great! Thanks for addressing them!