feat(table): add row, column buttons outside of table #6540
Conversation
|
Seems like a user interface change so please provide some screenshots ;) |
|
Yes, I will. It's not yet ready for review. |
Signed-off-by: Luka Trovic <[email protected]>
luka-nextcloud
force-pushed
the
table-tweaks
branch
from
ae3ed5a
to
1325ace
Compare
|
Screenshot & video demo.webm |
|
@nextcloud/designers Could you please give me your feedback regarding the button? :) |
| @@ -322,10 +322,10 @@ div.ProseMirror { | |||
|
|
|||
| table { | |||
| border-spacing: 0; | |||
| width: calc(100% - 50px); | |||
| width: calc(100% - 84px); | |||
There was a problem hiding this comment.
How about adding a comment about what 84px are and how they're compounded?
| table-layout: auto; | ||
| white-space: normal; // force text to wrapping | ||
| margin-bottom: 1em; | ||
| margin-bottom: 4em; |
There was a problem hiding this comment.
Same here. Also, I wonder why the button size is 34px on the side, but 3em (different unit) on the bottom. Maybe instead use calc(1em + 34px) instead?
| </NcButton> | ||
| <NcButton v-if="isEditable" | ||
| class="table-add-row" | ||
| :aria-label="t('text', 'Add row after')" |
There was a problem hiding this comment.
| :aria-label="t('text', 'Add row after')" | |
| :aria-label="t('text', 'Add row below')" |
Maybe better language-wise?
| const lastRowNode = this.node.lastChild | ||
| this.editor.chain() | ||
| .focus() | ||
| .setTextSelection(this.getPos() + this.node?.nodeSize - lastRowNode.nodeSize + 1) |
There was a problem hiding this comment.
The node? seems error-prone here. If this.node was faulty here, initialization of lastRowNode above would already fail, and it would probably lead to unwanted side-effects, no? If this.node can really be faulty here, I'd suggest to catch this properly and not do anything in this case.
| } | ||
| </script> | ||
|
|
||
| <style scoped lang="scss"> | ||
| .table-wrapper { | ||
| position: relative; | ||
| overflow-x: auto; | ||
|
|
||
| &.focused { |
There was a problem hiding this comment.
I'd expect the &.focused ruleset to be inside the .table-add-column and .table-add-row rulesets below, but that's rather nitpicking.
| @@ -58,6 +59,17 @@ export default TableCell.extend({ | |||
| } | |||
| }, | |||
|
|
|||
| renderHTML({ HTMLAttributes }) { | |||
| const attributes = mergeAttributes(this.options.HTMLAttributes, HTMLAttributes) | |||
There was a problem hiding this comment.
Could you elaborate a bit on why this is necessary? I didn't grasp what the colspan and rowspan attributes are used for before.
|
|
||
| &:hover { | ||
| opacity: 1; | ||
| visibility: unset; |
There was a problem hiding this comment.
In my understanding the visibility: unset is superfluous. The button already becomes visible on table focus, and it cannot be hovered when invisible: hidden anyway.
| @@ -4,7 +4,7 @@ | |||
| --> | |||
|
|
|||
| <template> | |||
| <NodeViewWrapper data-text-el="table-view" class="table-wrapper"> | |||
| <NodeViewWrapper data-text-el="table-view" class="table-wrapper" :class="{ 'focused': isFocused }"> | |||
There was a problem hiding this comment.
So far, the buttons only become visible when the table is focussed (i.e. the cursor is inside it). How about also making them visible when the table is hovered? I'd expect this from a user experience perspective.
📝 Summary
🖼️ Screenshots
🚧 TODO
🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)