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 table block column alignment #16111

Merged
merged 3 commits into from Jul 31, 2019

Conversation

@talldan
Copy link
Contributor

commented Jun 12, 2019

Description

Adds column alignment options to the table block. Closes #15823

Dev decisions
Code-wise, the part where I've had to break from the norm is by using a data-align attribute to store the alignment value for a cell. This is because the cell attributes are deeply nested and sourced using a query matcher, so normal attributes can't be used.

Applying an alignment adds a class name and data-align attribute to each table cell in a column. There aren't many alternatives to this approach that I can think of, apart from using inline styles. colgroup/col elements don't support text align, so they're out of the question.

How has this been tested?

Added unit and e2e tests.

Manual testing steps:

  1. Add a table block to a post
  2. Add some text to the first column of the table.
  3. Ensure one of the cells in the first column is selected
  4. From the toolbar try each of the column alignment options
  5. Verify that text in the each cell in the column is aligned in the editor
  6. Preview the post
  7. Verify that the text is aligned when viewing the post.

Screenshots

Editor
Screen Shot 2019-07-30 at 10 46 50 am

Post
Screen Shot 2019-07-30 at 10 47 45 am

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@mapk

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

I just tested this and it works wonderfully! Thanks, @talldan! 👍 from a design perspective. :shipit:

Screen Shot 2019-06-15 at 7 18 06 AM

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Thanks! I think I'll switch this to using classnames as #16035 does, as that's now been merged.

@mtias mtias added this to the Gutenberg 6.1 milestone Jun 25, 2019

@mtias

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Should this apply to the whole table, per column, or on individual cells?

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Should this apply to the whole table, per column, or on individual cells?

Great question, @mtias. Right now the block only allows selection of a single cell at a time. I would love to see selections happen across rows and columns. Once this works, we can look at alignment across these.

@talldan talldan force-pushed the add/table-block-cell-alignment branch from 61f19ed to 7fdde45 Jun 27, 2019

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Right now the block only allows selection of a single cell at a time. I would love to see selections happen across rows and columns. Once this works, we can look at alignment across these.

This is my take on it as well.

I've refactored this to use classes, so it's ready for a code review 👍

@talldan talldan force-pushed the add/table-block-cell-alignment branch from 7fdde45 to e4cdc2f Jul 1, 2019

@youknowriad
Copy link
Contributor

left a comment

This seesm to be in a good shape. I left some minor comments.

"align": {
"type": "string",
"source": "attribute",
"attribute": "data-align"

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 4, 2019

Contributor

Interesting. since it applied as a class, makes me wonder if at some point, we'd want "classNames" sources. That said, it can be complex and I know it's a deliberate decision to restrict the number of sources available. css @aduth

This comment has been minimized.

Copy link
@aduth

aduth Jul 9, 2019

Member

Yeah, I think we'd considered once before a source serialized as the presence of a class name. Personally I don't have much of an issue with the data- attributes; they can be styled just as well, are a bit easier to parse, and fit well as serving the purpose of a data attribute.

Custom data attributes are intended to store custom data, state, annotations, and similar, private to the page or application, for which there are no more appropriate attributes or elements.

https://html.spec.whatwg.org/multipage/dom.html#embedding-custom-non-visible-data-with-the-data-*-attributes

That being said, I'm not really sure why we bother to serialize this in the HTML at all, if we're not using it for anything like styling? Why not just embed it within the comment delimiters (i.e. omit source)?

This comment has been minimized.

Copy link
@talldan

talldan Jul 12, 2019

Author Contributor

@aduth As this attribute is in a query, my understanding is that source can't be omitted. A classNames source could be useful in this use case.

An argument against data- attributes is that it's double storing data, but then it's not too much of a problem with the reactive nature of blocks, it's not like there's extra business logic to keep them in sync.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 30, 2019

Member

Why use the class at all? Can we not just use the data attribute?

This comment has been minimized.

Copy link
@talldan

talldan Jul 30, 2019

Author Contributor

There are some global styles for text alignment:

// Text alignments.
.has-text-align-center {
text-align: center;
}
.has-text-align-left {
text-align: left;
}
.has-text-align-right {
text-align: right;
}

Seems better to be able to use those and not have extra styles in the table block.

packages/block-library/src/table/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/table/style.scss Outdated Show resolved Hide resolved
@ellatrix
Copy link
Member

left a comment

I have some strong opinion about the user experience of this, sorry.

  1. We should only allow table cells to be left- or right-aligned. The button can be a simple toggle. Center alignment in tables doesn't make so much sense. What kind of data will you align like that? The general rule of thumb is that textual data should be left-aligned, and numerical data should be right-aligned.
  2. Another table "rule" is that each column should have the same alignment. This not only looks better, it easier for the user to align the entire column at once.
@mtias

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

I think center alignment can be perfectly fine in some cases, and we should not dictate that. However, I think that single cell alignment is a bit excessive. I'd rather have a text alignment toolbar button that applies to the whole table at once (since columns is a bit more complicated).

@talldan talldan removed the Needs Decision label Jul 30, 2019

@talldan talldan modified the milestones: Gutenberg 6.2, Gutenberg 6.3 Jul 30, 2019

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

I've updated this PR so that it now aligns entire columns.

Tests are all green (but a github action failed), so it should be ready for review.

@gziolo gziolo removed this from the Gutenberg 6.3 milestone Jul 30, 2019

*
* @return {boolean} True if the cell is selected, false otherwise.
*/
export function isCellSelected( cellLocation, selection ) {

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 30, 2019

Member

Why not have isCellSelected and isColumnSelected?

This comment has been minimized.

Copy link
@talldan

talldan Jul 30, 2019

Author Contributor

isColumnSelected would only be used in one place, so it seems unnecessary at this stage to have two separate functions.

I'm exploring this more in #16493, where I do have separate functions for different types of selections as well as the same isCellSelected for easily checking whether a given cell is in any type of selection:

/**
* Determines if a cell is selected.
*
* @param {Object} cellLocation The cell to check.
* @param {Object} selection The selection data.
*
* @return {boolean} True if the cell is within a selection, false otherwise.
*/
export function isCellSelected( cellLocation, selection ) {
if ( ! cellLocation || ! selection ) {
return false;
}
switch ( selection.type ) {
case 'cell':
return hasSingleCellSelection( cellLocation, selection );
case 'table':
return true;
case 'row':
return hasRowSelection( cellLocation, selection );
case 'column':
return hasColumnSelection( cellLocation, selection );
}
}

It'll be an easy refactor from this to that.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

That's fine, but then I wouldn't call it isCellSelected? That's misleading if you're checking if a column is selected. isCellSelected( ..., 'column' ) doesn't make sense?

This comment has been minimized.

Copy link
@talldan

talldan Jul 31, 2019

Author Contributor

Hmm, yeah, that's good feedback. I can work on a few naming improvements in future PRs as this functionality develops. Something like isCellWithinSelection might be more appropriate.

Thanks for the reviews btw!

} ) ),
cells: times( cellCount, ( index ) => {
const firstCellInColumn = get( firstRow, [ 'cells', index ], {} );
const inheritedAttributes = pick( firstCellInColumn, INHERITED_COLUMN_ATTRIBUTES );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 30, 2019

Member

Why is alignment inherited?

This comment has been minimized.

Copy link
@talldan

talldan Jul 30, 2019

Author Contributor

When adding a new row, the newly added cells will keep the same alignment as the rest of their adjacent column.

As already mentioned elsewhere in this PR, it seems like all cells in a column should have the same alignment, so this maintains that experience for the user. In my testing it felt like the right way to go.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

Sounds good

rowIndex,
columnIndex,
content,
attributeName,

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 30, 2019

Member

I think this would read better if attributeName is a separate argument. E.g. getCellAttribute( state, 'align', cell ).

This comment has been minimized.

Copy link
@talldan

talldan Jul 31, 2019

Author Contributor

Agreed it could be improved. I've changed it to getCellAttribute( state, cell, 'align' ).

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

Great! It seems like a good idea to keep cellLocation as a separate entity. Could this object be documented somewhere to the type can be used in the docs of all these functions?

setAttributes( updateSelectedCell(
attributes,
selectedCell,
( cellAttributes ) => ( { ...cellAttributes, content } ),

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 30, 2019

Member

Why do we need a callback here? Can this not be done directly by updateSelectedCell?

This comment has been minimized.

Copy link
@talldan

talldan Jul 30, 2019

Author Contributor

It might be overkill, but I feel it's a more useful API. Being able to read the cell's attributes provides an opportunity for doing things like toggling values or incrementing/decrementing values when updating them.

@paaljoachim

This comment has been minimized.

Copy link

commented Jul 30, 2019

I assume one can then multi select cells in a column and have all of them align in one go.

@talldan talldan force-pushed the add/table-block-cell-alignment branch from 72a11ec to 1dc0a87 Jul 31, 2019

talldan added 3 commits Jun 12, 2019
Initial step at adding alignment options in table block edit component
Fix block.json attributes

Add cell alignment to save function

Add tests for table cell alignment

Tidy up test comments

Update JSDocs for new functions

Add unit tests

Use classnames for cell alignment

Use shorthand property

Remove alignment styles in favour of global styles

Update copy of alignment buttons

Rework cell selection border to use an actual border around a pseudoelement instead of box-shadow
Make cell alignment work across columns instead of individual cells
Add jsdocs

Inherit the alignment property of the first cell in the column when adding new rows

Update e2e tests

Use consistent letter casing
Address review feedback
Update params of getCellAttribute function

Do nothing if insertRows is unable to determine the correct cellCount for the insertion

@talldan talldan force-pushed the add/table-block-cell-alignment branch from 1dc0a87 to f00dd4e Jul 31, 2019

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

I assume one can then multi select cells in a column and have all of them align in one go.

@paaljoachim Sort of, the table still only has cell-based selection, but alignments apply to the column that the selected cell is in.

@talldan talldan requested a review from ellatrix Jul 31, 2019

*/
async function changeCellAlignment( align ) {
await clickBlockToolbarButton( 'Change column alignment' );
const alignButton = await page.$x( `//button[text()='Align Column ${ capitalize( align ) }']` );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

Can the clickButton utility function be used here?

This comment has been minimized.

Copy link
@talldan

talldan Jul 31, 2019

Author Contributor

Genuinely didn't know that existed. Will do a follow up PR to tidy up the tests.

await page.keyboard.type( '4' );

// Create the table.
const [ createButton ] = await page.$x( createButtonSelector );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 31, 2019

Member

Also here, could clickButton be used?

@talldan talldan merged commit c59cda8 into master Jul 31, 2019

1 of 5 checks passed

Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@talldan talldan deleted the add/table-block-cell-alignment branch Jul 31, 2019

@talldan talldan added this to the Gutenberg 6.3 milestone Aug 1, 2019

@talldan talldan referenced this pull request Aug 2, 2019
5 of 5 tasks complete
gziolo added a commit that referenced this pull request Aug 29, 2019
Add table block column alignment (#16111)
* Initial step at adding alignment options in table block edit component

Fix block.json attributes

Add cell alignment to save function

Add tests for table cell alignment

Tidy up test comments

Update JSDocs for new functions

Add unit tests

Use classnames for cell alignment

Use shorthand property

Remove alignment styles in favour of global styles

Update copy of alignment buttons

Rework cell selection border to use an actual border around a pseudoelement instead of box-shadow

* Make cell alignment work across columns instead of individual cells

Add jsdocs

Inherit the alignment property of the first cell in the column when adding new rows

Update e2e tests

Use consistent letter casing

* Address review feedback

Update params of getCellAttribute function

Do nothing if insertRows is unable to determine the correct cellCount for the insertion
gziolo added a commit that referenced this pull request Aug 29, 2019
Add table block column alignment (#16111)
* Initial step at adding alignment options in table block edit component

Fix block.json attributes

Add cell alignment to save function

Add tests for table cell alignment

Tidy up test comments

Update JSDocs for new functions

Add unit tests

Use classnames for cell alignment

Use shorthand property

Remove alignment styles in favour of global styles

Update copy of alignment buttons

Rework cell selection border to use an actual border around a pseudoelement instead of box-shadow

* Make cell alignment work across columns instead of individual cells

Add jsdocs

Inherit the alignment property of the first cell in the column when adding new rows

Update e2e tests

Use consistent letter casing

* Address review feedback

Update params of getCellAttribute function

Do nothing if insertRows is unable to determine the correct cellCount for the insertion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.