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

Feature/114/tables #2235

Merged
merged 26 commits into from
Mar 31, 2022
Merged

Feature/114/tables #2235

merged 26 commits into from
Mar 31, 2022

Conversation

max-nextcloud
Copy link
Collaborator

@max-nextcloud max-nextcloud commented Mar 14, 2022

src/nodes/Table.js Outdated Show resolved Hide resolved
@max-nextcloud
Copy link
Collaborator Author

max-nextcloud commented Mar 14, 2022

This is still very basic... obviously a bunch of things are still missing:

  • basic styling for the tables - they are currently only showing bottom borders which might be too little when they are empty
  • table head that looks different from cells
  • saving table with two empty lines at the end merges these lines in markdown
  • tab key is not navigating between table headings
  • fix table modification functions
    • Fix addRowAfter function. Pressing tab on the last cell should create a new row. Currently it tries to but fails because our table schema is different
    • fix the other table modification functions as well.

I'm not sure how to design the ui. Currently i am thinking a single table button that creates a table and if the table is active clicking this button will open a submenu rather than toggling off the table. The submenu would then contain the buttons to add / remove columns / rows etc.

@juliusknorr
Copy link
Member

juliusknorr commented Mar 15, 2022

Quick early design feedback form the spontaneous design co-working:

  • for the header, only the bottom border should be darker, possibly no background color
  • there should be a border radius on the table
  • some margin at the bottom would help to separate from the next block
  • keyboard actions
    • enter in the last row should create a new row
    • tab navigation should work within cells, not creating new rows or columns
  • https://materialdesignicons.com/icon/table might be a nicer icon
  • when the table is focussed, buttons could be shown for adding/removing rows/columns outside of the table (ref Notion.so)
  • 3-dot actions could be shown on the right in the column header cells with a menu having: "Add column to right", "Add column to left", "Delete column".

Screenshot 2022-03-15 at 14 44 35

@max-nextcloud
Copy link
Collaborator Author

max-nextcloud commented Mar 16, 2022

Enter already creates a new row almost everywhere in the table.
It does not work in the last cell of a row though and not if the cell contains text.

  • make it navigate to the next row everywhere but in the last row
  • make it create a new row in the last row - even if the cell has content
  • make it create a new row in the last row even in the last cell.

@max-nextcloud max-nextcloud force-pushed the feature/114/tables branch 2 times, most recently from 9b9bd53 to 0ad82d9 Compare March 17, 2022 12:43
@max-nextcloud

This comment was marked as outdated.

@max-nextcloud max-nextcloud force-pushed the feature/114/tables branch 3 times, most recently from 0211ad1 to 2254f0c Compare March 22, 2022 13:20
@max-nextcloud
Copy link
Collaborator Author

max-nextcloud commented Mar 23, 2022

What's missing to get this ready to merge:

  • fix markdown unit tests
  • fix cypress tests by telling cypress how to handle vue files (possibly turn this into a component test)
  • add button to delete the table
  • rebase & compile

Further tweaks that would be nice to have but could be fixed separately

  • use color-primary-light for highlighting current row

Moved to a separate issue #2267 :

  • check if rows and columns can be added / removed and disable buttons accordingly
  • refactor test layout, put all tables fixtures in one folder
  • accessibility for adding columns and rows (currently tab moves past the buttons in the cells)
  • drop colspan=1, rowspan=1, etc from rendered html - first attempt at this failed.

@max-nextcloud max-nextcloud force-pushed the feature/114/tables branch 3 times, most recently from 3fa26b0 to 3c8f7b2 Compare March 23, 2022 13:31
@max-nextcloud max-nextcloud marked this pull request as ready for review March 23, 2022 13:32
@max-nextcloud max-nextcloud requested review from a team, vinicius73, mejo-, julien-nc, juliusknorr and luka-nextcloud and removed request for a team March 23, 2022 13:32
@max-nextcloud
Copy link
Collaborator Author

max-nextcloud commented Mar 23, 2022

I think this is good (enough) to go. I'd be curious what you think.
I'll see if i can get to fix some of the open tasks left while addressing other things that come up during a review. Otherwise i'd move them to a new issue and fix them later after this has landed.

update: that was before i noticed all cypress tests are failing 😭

@max-nextcloud
Copy link
Collaborator Author

Looks like the test failures are not related to this branch... they also happen on master.

@mejo-
Copy link
Member

mejo- commented Mar 23, 2022

Wow, looks super nice 🤩

I only gave it a quick try and it looks really great! Some glitches I experienced so far:

  • Adding a table at the very top of the document, I'm no longer able to put something on top if it afterwards. Would be nice if I could put the cursor above the table somehow
  • Navigating with the arrow up (or left) key from below the table, the cursor ends up on the right side of the table. I'm able to add content there. That's unexpected and I'd consider it unwanted ;)
  • Further navigating up with the arrow up keys, the lower right field first is reached first and then it goes up the very right column until the last body field. I'm not able to jump into the header field by navigating with the cursors.
  • If the table is the first node in my document and I try to navigate further up from a header field or the last body field, the cursor jumps below the table.

@max-nextcloud created #2266 to track this.

@juliusknorr juliusknorr added this to the Nextcloud 24 milestone Mar 24, 2022
max-nextcloud and others added 25 commits March 31, 2022 14:29
Each node is responsible for rendering its markdown content.

This splits the responsibility and simplifies the `toMarkdown` functions a lot.
At the same time it makes it harder to beautify the entire table
because every cell only knows about itself - not about the rest of the column.

To work around this we can introduce padding attributes for TableCell and TableHeader.
These need to be updated when the content of the table changes.
They will also allow us to preserve the original padding from a markdown file.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Also make sure that table rows are rendered on one line each.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Prosemirrors `goToNextCell` command checks for a node with tableRole `row`.
Without this the command fails.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Prosemirror expects colspan, rowspan and the like to calculate the table layout.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Only create a new row when we have reached the end of the table.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
There is enough space already due to the size of the actions button.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
little bit of padding on the top of the text so it is in the middle of the cell
on single line cells.

Use icons that are better destinguishable for delete commands.

Signed-off-by: Max <max@nextcloud.com>
no idea why but it seems to be undefined.
Reported upstread here: cypress-io/github-action#524

Signed-off-by: Max <max@nextcloud.com>
Hide it in table settings so it is harder to hit by accident.
Undo still works - but still - it is quite a destructive action.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud merged commit 2a26764 into master Mar 31, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/114/tables branch March 31, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Markdown table support
5 participants