Adds fixed table layout to table block #18870
Conversation
I have added this to both the editor and also to the front styles. It might be a point for discussion along with should this be done, but also would the editor and theme show it? Fixes #16045
karmatosed
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
|
Hey @karmatosed - what are your thoughts on the existing option for Fixed Table Cells: Is that something you think could be removed? |
|
@talldan I do and let me remove that from the PR. |
| @@ -6,6 +6,7 @@ | |||
| height: auto; | |||
|
|
|||
| table { | |||
| table-layout: fixed; | |||
There was a problem hiding this comment.
Isn't this unnecessary since style.scss is loaded in the editor?
There was a problem hiding this comment.
In testing it was for me, feel free to iterate on PR if you don't find it is in testing @ZebulanStanphill
There was a problem hiding this comment.
I think @ZebulanStanphill is right, it shouldn't be required.
This builds on previous PR and removes option for fixed width, as it's now default.
karmatosed
requested review from
chrisvanpatten,
nerrad,
ntwb and
youknowriad
as code owners
karmatosed
removed
the
Needs Decision
There was a problem hiding this comment.
@karmatosed This looks pretty close, a few things around the tests and deprecations might need a bit of extra work. Let me know if you'd like me to take a look at those few items.
| @@ -15,10 +15,6 @@ const supports = { | |||
| const deprecated = [ | |||
| { | |||
| attributes: { | |||
| hasFixedLayout: { | |||
There was a problem hiding this comment.
The fixed layout option shouldn't be removed from the block deprecation, it's important it remains so that old versions of the block don't receive a validation error.
| theme(button) 72% | ||
| ); | ||
| background-image: | ||
| linear-gradient(-45deg, theme(button) 28%, color(theme(button) shade(20%)) 28%, color(theme(button) shade(20%)) 72%, theme(button) 72%); |
There was a problem hiding this comment.
Looks like this shouldn't have been updated given the comment above.
| @@ -4,7 +4,6 @@ | |||
| "name": "core/table", | |||
| "isValid": true, | |||
| "attributes": { | |||
| "hasFixedLayout": false, | |||
There was a problem hiding this comment.
This should remain so that the deprecation is tested correctly.
| @@ -26,7 +25,6 @@ export default function save( { attributes } ) { | |||
| const backgroundClass = getColorClassName( 'background-color', backgroundColor ); | |||
|
|
|||
| const classes = classnames( backgroundClass, { | |||
| 'has-fixed-layout': hasFixedLayout, | |||
There was a problem hiding this comment.
Will have to test whether this change to the save output requires a further block deprecation to be added. Often if it's only a class name change it doesn't seem to require a deprecation.
| @@ -18,11 +18,6 @@ exports[`Table allows adding and deleting columns across the table header, body | |||
| <!-- /wp:table -->" | |||
| `; | |||
|
|
|||
| exports[`Table allows cells to be selected when the cell area outside of the RichText is clicked 1`] = ` | |||
There was a problem hiding this comment.
The snapshot shouldn't have been removed as the test is still present. Instead it needs to be updated.
| @@ -6,6 +6,7 @@ | |||
| height: auto; | |||
|
|
|||
| table { | |||
| table-layout: fixed; | |||
There was a problem hiding this comment.
I think @ZebulanStanphill is right, it shouldn't be required.
|
@talldan what do you think about closing this as it's sat so long? I can always close this and we could start a fresh PR, if easier. |
|
Closing this out -- welcome folks to open a new one to keep it fresh, as said above! |
I have added this to both the editor and also to the front styles. It might be a point for discussion along with should this be done, but also would the editor and theme show it?
Fixes #16045