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

Add/147 table block (alternate vision) #850

Merged
merged 52 commits into from
Jul 3, 2017
Merged

Conversation

BoardJames
Copy link

The purpose of the pull request is to:

  • Provide an alternate vision of how a table block could be implemented (it is "Table 2" in the add menu).
  • Use TinyMCE (and table plugin) to do the work rather than reimplementing everything in react

TinyMCE already provides execCommands to add columns, rows, make headings, and merge cells - I think we should be using them to make the implementation easier and more robust.

Note I have used the icons from the TinyMCE inlite theme for the table operations. I imagine they will have to be replaced before this is considered for merging by a more official icon but I couldn't find anything appropriate.

@mtias mtias added the [Feature] Blocks Overall functionality of blocks label May 22, 2017
@BoardJames
Copy link
Author

I moved the controls into a menu
tabletoolbarmenu

@BoardJames
Copy link
Author

Added the alignment controls and matched the CSS of the existing table:
tablewithalignmentmodes

@ellatrix
Copy link
Member

While this approach would definitely be the faster way to implement all the table controls, I do like to try to keep Editable small and inline. I don't think it's hard or impossible to implement all the controls like that, it's just code that is now in the table plugin that would be in this block registration, or possibly some more abstracted behaviour. In #62 block selection looks very much like cell selection.

@BoardJames
Copy link
Author

I do like to try to keep Editable small and inline.

I do not understand this criticism - I have not changed Editable?

@nylen
Copy link
Member

nylen commented May 26, 2017

After #849 which added a bunch of tests for block parsing and serialization, this PR should probably be rebased against master, either because the build will fail upon merge, or because this PR can add some new tests using this structure (details here).

@jasmussen
Copy link
Contributor

From the screenshot alone I think this should be merged in for sure.

It's missing a fullwide alignment option but that's trivial to add and probably something we should do separately!

@youknowriad can you take a quick look at the code so we can get this in? Thanks both!

@BoardJames
Copy link
Author

It's missing a fullwide alignment option

I copied the alignment options off the existing table.

@BoardJames
Copy link
Author

BoardJames commented Jun 30, 2017

I tried out align "full", it seems a bit glitchy for tables:
align-full-glitchy

Notably the up/down arrows get pushed off the screen and the horizontal scrollbar is shown.

@jasmussen
Copy link
Contributor

I think it's easy to fix the hoz scrollbar, and add the fullwide. But also worth doing separately, it's something I think I can do. So not something that should hold up this PR.

}
},

edit( { attributes, setAttributes, focus, setFocus } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a new className prop we should add to the root element of the edit function (see other blocks).

Copy link
Author

Choose a reason for hiding this comment

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

I have propagated this prop to the editable.

@@ -0,0 +1,36 @@
.editor-visual-editor__block[data-type="core/table2"] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should encourage the use of the new generated classNames for styling wp-block-table2

Copy link
Author

@BoardJames BoardJames Jul 3, 2017

Choose a reason for hiding this comment

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

Ok, I have moved the table styles so they are a child of wp-block-table-2. I can't move the other styles though because the div with wp-block-table-2 class is not the same div that is selected by .editor-visual-editor__block[data-type="core/table2"].

/>,
focus && (
<BlockControls key="menu">
<ToolbarMenu
Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand correctly, the purpose of this component is to display a toolbar with a dropdown button.
What if I want to display several dropdown buttons in the same toolbar?

I think instead we'd probably want to refactor this to be able to do something like:

<Toolbar>
  <DropdownMenu />
  <DropdownMenu />
</Toolbar>

Copy link
Author

Choose a reason for hiding this comment

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

I have changed it so it nests inside a Toolbar and renamed it to DropdownMenu.

@youknowriad
Copy link
Contributor

This block works great, I left some small notes but if @jasmussen thinks we should get this in today's release we can merge and resolve this in a follow-up PR.

@jasmussen
Copy link
Contributor

Whether it goes in todays release or not is less important, but I think it would be good to merge before the PR gets any bigger, and then open smaller PRs to fix any remaining issues.

@BoardJames BoardJames merged commit 7872ce5 into master Jul 3, 2017
@BoardJames BoardJames deleted the add/147-table-block branch July 3, 2017 06:28
@BoardJames
Copy link
Author

I addressed the feedback and merged it.

@jasmussen
Copy link
Contributor

Hooray! Thanks for working on this James!

@joedolson
Copy link
Contributor

One of the original stated reasons for using the TinyMCE table plugin was the ability to make headings - it's a crucial element for accessibility for this block to support the ability to define a heading. See #1470

@BoardJames
Copy link
Author

@joedolson I will add table headings to my TODO list. It might require some changes to the table plugin to implement.

@StaggerLeee
Copy link

Tables are now very, very usable. For the first time in WP visual editor.
If it not much coding troubles for you think about already mentione drag & drop columns/rows.

People tend to fill cells, rows then they realise they filled wrong one. Happens often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.