Adding border to table header & footer #18767 #19136
Conversation
dmahendrakar
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
|
@ajitbohra I see there are no failing tests after after my change. Do you need me to add a snapshot test for table component? I see it's only added in block-library/embed. Is there any reason why snapshot tests are not added pervasively? |
|
@talldan do you have any feedback on this pr? |
|
I like it. I wonder if it would stand out just a little more at 3px? |
|
@shaunandrews this is how it looks 2px vs 3px - |
|
I tried it out with the Gutenberg Starter Theme (https://github.com/WordPress/gutenberg-starter-theme): At the moment the style is only applied in the editor, and I think the styles should be consistent when also viewing the post. @dpkm95 To do that, these styles could be added to the table block's theme.scss file, which should apply to both the editor and the post. That file is generally for slightly more opinionated styles that are optional for themes to use. |
talldan
added
[Block] Table
|
@talldan thanks for the feedback. Do you like 2px or 3px border? +@shaunandrews. [0] [1] [2] |
I have a slight preference for the 3px visually. Maybe others have differing opinions, I'll get more eyes. |
|
A few more people chimed in on Slack — lets go with 3px. |
|
@dpkm95 As mentioned above, this should go in the theme.scss stylesheet, not the style.scss stylesheet. In terms of Twenty Twenty, it was a surprise to me, but it looks like that theme doesn't support default block styles: Any changes added to the theme.scss won't be visible when viewing a post in Twenty Twenty. It's up to the theme to add styling as it sees fit. My recommendation would be to use a different theme to test this feature. Generally a good one to use is the Gutenberg Starter Theme (https://github.com/WordPress/gutenberg-starter-theme), as it's more of a base theme than other themes. You can download it from the repo using the Twenty Nineteen also seems to work fine. I also tried a few older themes, but they seem to have |
|
@talldan thanks for the clarification, It works for me with gutenberg-starter-theme! I've moved the styles to theme.scss, can you please review the pr. |
|
@dpkm95 Looking good! What do you think about also adding the placeholder text for the header and footer cells as described in this comment - #18767 (comment)? |
|
I've updated pr with placeholder. Thanks for the review! This is how it looks @shaunandrews @talldan - |
| @@ -450,6 +450,13 @@ export class TableEdit extends Component { | |||
| [ `has-text-align-${ align }` ]: align, | |||
| }, 'wp-block-table__cell-content' ); | |||
|
|
|||
| let placeholder = ''; | |||
| if ( name === 'head' ) { | |||
| placeholder = 'Header label'; | |||
There was a problem hiding this comment.
This string should be translatable.
| if ( name === 'head' ) { | ||
| placeholder = 'Header label'; | ||
| } else if ( name === 'foot' ) { | ||
| placeholder = 'Footer label'; |
There was a problem hiding this comment.
This string should also be translatable.
Thanks @talldan looking forward to add more contributions! |
|
latest pr: #19450 with correct github user & email |
Description
Added 2px border-bottom to header and border-top to footer. Design suggested by @shaunandrews in #18767
Closes #18767.
How has this been tested?
Screenshots
Types of changes
CSS changes limited to editor view.
Checklist: