When an Olivero table has a row header, the alignment of row header text is awkward and last row border is confusing [#3420865]

Problem/Motivation

Olivero table styles are awkward when the table has a row header element. Also the last row border looks broken.

Steps to reproduce

Create an article node with this HTML:

<table>
    <thead>
        <tr>
            <th>
                Header
            </th>
            <th>
                Header
            </th>
            <th>
                Header
            </th>
        </tr>
    </thead>
    <tbody>
        <tr>
            <th>
                Also header
            </th>
            <td>
                Content
            </td>
            <td>
                And more
            </td>
        </tr>
        <tr>
            <th>
                Also header
            </th>
            <td>
                Also content
            </td>
            <td>
                As well here
            </td>
        </tr>
        <tr>
            <th>
                Final header
            </th>
            <td>
                Final row
            </td>
            <td>
                And last cell
            </td>
        </tr>
    </tbody>
</table>

Proposed resolution

Header cells in not the first row should not have the padding-bottom --sp set IMHO, this landing on the same line as the rest of the cells.
Header cells in the last row of the table should not have the bottom border.

Remaining tasks

~~- Fix~~
~~- Review~~
- Commit

User interface changes

Before

before

After

after

API changes

None

Data model changes

None

Release notes snippet

None

Comment	File	Size	Author
#49	3420865-nr-bot.txt	1.26 KB	needs-review-queue-bot
#42	3420865-nr-bot.txt	90 bytes	needs-review-queue-bot
#41	after MR.png	45.3 KB	pemanamgay
#41	before MR.png	37.24 KB	pemanamgay
#35	afterpatch_6566.png	74.11 KB	kanchan bhogade
#35	beforepatch_6566.png	73.11 KB	kanchan bhogade
#32	Screenshot 2024-02-22 at 9.15.58 AM.png	348.17 KB	gauravvvv
#24	afterpatch.png	18.82 KB	kanchan bhogade
#24	beforepatch.png	19.34 KB	kanchan bhogade
#23	After-patch.png	47.81 KB	Rangaswini
#23	Before-patch.png	51.95 KB	Rangaswini
#18	After patch.png	45.61 KB	Rangaswini
#18	Before patch.png	48.87 KB	Rangaswini
#9	Screenshot at Feb 13 11-01-47.png	1.4 MB	shweta__sharma
	Screenshot 2024-02-12 at 15.53.35.png	103.81 KB	gábor hojtsy

Issue fork drupal-3420865

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3420865-when-an-olivero changes, plain diff MR !6566
Check out this branch for the first time

Check out existing branch, if you already have it locally

1 hidden branch

3420865-Olivero-table-head

changes, plain diff MR !7718

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

12 February 2024 at 14:54

Gábor Hojtsy created an issue. See original summary.

Comment #2

gábor hojtsy

he/him

Hungarian

Hungary

commented 12 February 2024 at 14:56

Issue summary:

View changes

Comment #3

13 February 2024 at 04:03

Gauravvvv made their first commit to this issue’s fork.

Comment #4

13 February 2024 at 04:19

Gauravvvv opened merge request !6566

Comment #5

gauravvvv commented 13 February 2024 at 04:19

Status:

Active

» Needs review

Comment #6

Rangaswini commented 13 February 2024 at 04:38

Assigned:

Unassigned

» Rangaswini

Comment #7

nitin shrivastava commented 13 February 2024 at 04:53

Status:	Needs review	» Needs work
Issue tags:		+Needs issue summary update

Please mention steps to reproduce, currently, it is not clear where we can find them.

Comment #8

shweta__sharma commented 13 February 2024 at 05:14

Issue tags:

+Needs steps to reproduce

Comment #9

shweta__sharma commented 13 February 2024 at 05:36

Issue summary:	View changes
Issue tags:	-Needs issue summary update

Status	File	Size
new	Screenshot at Feb 13 11-01-47.png	1.4 MB

I have updated the IS and added STR but not sure how to make the first row and first column both headers. So I am not removing the STR tag.

Comment #10

shweta__sharma commented 13 February 2024 at 05:37

Issue summary:

View changes

Comment #11

shweta__sharma commented 13 February 2024 at 05:57

Pushed the code for the alignment issue in table.

Thanks

Comment #12

gábor hojtsy

he/him

Hungarian

Hungary

commented 13 February 2024 at 06:43

Issue summary:	View changes
Issue tags:	-Needs steps to reproduce

Creating a view to reproduce this is super complicated. I merely created a table in ckeditor. Added the resulting markup for easier testing into the issue summary.

Comment #13

gábor hojtsy

he/him

Hungarian

Hungary

commented 13 February 2024 at 06:44

Issue summary:

View changes

Comment #14

gábor hojtsy

he/him

Hungarian

Hungary

commented 13 February 2024 at 06:46

Issue summary:

View changes

Comment #15

shweta__sharma commented 14 February 2024 at 04:34

Status:

Needs work

» Needs review

Updating the status needs review as nothing is left to do in this.

Comment #16

shweta__sharma commented 14 February 2024 at 05:03

Assigned:

Rangaswini

» Unassigned

Comment #17

Rangaswini commented 14 February 2024 at 05:06

Assigned:

Unassigned

» Rangaswini

Comment #18

Rangaswini commented 14 February 2024 at 05:19

Status:

Needs review

» Reviewed & tested by the community

Status	File	Size
new	Before patch.png	48.87 KB
new	After patch.png	45.61 KB

Applied the patch and last row border is removed now but row alignment is still there, adding screenshots below

Comment #19

Rangaswini commented 14 February 2024 at 05:23

Assigned:

Rangaswini

» Unassigned

Comment #20

shweta__sharma commented 14 February 2024 at 05:43

Status:

Reviewed & tested by the community

» Needs work

Comment #21

shweta__sharma commented 14 February 2024 at 05:54

Status:

Needs work

» Needs review

Comment #22

Rangaswini commented 14 February 2024 at 06:06

Assigned:

Unassigned

» Rangaswini

Comment #23

Rangaswini commented 14 February 2024 at 06:19

Assigned:	Rangaswini	» Unassigned
Status:	Needs review	» Reviewed & tested by the community

Status	File	Size
new	Before-patch.png	51.95 KB
new	After-patch.png	47.81 KB

LGTM, applied patch for removing padding which is causing issue for alignment and it is resolved now. Please refer screenshots

Comment #24

kanchan bhogade commented 14 February 2024 at 07:15

Status	File	Size
new	beforepatch.png	19.34 KB
new	afterpatch.png	18.82 KB

Hi
Verified and tested MR !6566 on Drupal 11 for the Olivero theme
Applied patch successfully...

Testing Steps

Install drupal version
Install the Olivero theme and set it as the default
Add table and check for last row border and row alignment
Apply the patch and check for the same

Test Result:
The last row border is removed and the row alignment issue is fixed

Attaching screenshot for reference
RTBC+1

Comment #25

shweta__sharma commented 14 February 2024 at 07:35

Issue summary:

View changes

Comment #26

gábor hojtsy

he/him

Hungarian

Hungary

commented 14 February 2024 at 09:11

The changes visually look good, thanks! I have no idea if this is the best way to implement them in Olivero's CSS :)

Comment #27

nod_

French

Lille

commented 20 February 2024 at 09:18

Comment #28

nod_

French

Lille

commented 20 February 2024 at 09:21

Status:

Reviewed & tested by the community

» Needs work

Comment #29

20 February 2024 at 11:31

karanpagare made their first commit to this issue’s fork.

Comment #30

karanpagare commented 20 February 2024 at 11:37

Status:

Needs work

» Needs review

Updated MR to use logical properties, can review.

Comment #31

smustgrave commented 20 February 2024 at 20:33

Status:	Needs review	» Needs work
Issue tags:		+Needs screenshots

As a UI change the proposed solution should have before/after screenshots in the issue summary (where its says TBD)

Comment #32

gauravvvv commented 22 February 2024 at 03:48

Issue summary:	View changes
Status:	Needs work	» Reviewed & tested by the community
Issue tags:	-Needs screenshots

Status	File	Size
new	Screenshot 2024-02-22 at 9.15.58 AM.png	348.17 KB

Added before and after patch screenshots. Changes looks good to me. Border bottom is removed, also alignment is fixed. Moving to RTBC.

Comment #33

nod_

French

Lille

commented 22 February 2024 at 10:43

Status:

Reviewed & tested by the community

» Needs work

Have a question about the vertical align change. It's pretty overkill. I think we should adjust the fontsize/padding of th in the table body instead. Like this we'll mess up layout for tables with cells that are not the same length and it could make them very hard to read.

Comment #34

gauravvvv commented 23 February 2024 at 03:27

Status:

Needs work

» Needs review

Addressed feedback of #33, updated MR for same.

Comment #35

kanchan bhogade commented 23 February 2024 at 07:50

Status	File	Size
new	beforepatch_6566.png	73.11 KB
new	afterpatch_6566.png	74.11 KB

Hi
I've tested the Updated MR- MR !6566 on Drupal version 11.x
The patch applied successfully...

Test Result:
Font size and padding of the table heading are updated and visually also looks good

attaching screenshots for reference

RTBC+1

Keeping in "Needs review" for code verification and more reviews

Comment #36

nod_

French

Lille

commented 23 February 2024 at 07:56

Status:	Needs review	» Needs work
Issue tags:		+Needs subsystem maintainer review, +Needs issue summary update

need some validation from olivero maintainers for the size change.

Need to update the screenshots in the issue summary as well.

Comment #37

karanpagare commented 28 February 2024 at 06:47

Updated as per @nod_ to use px instead of rem. Rest can be reviewed by Olivero maintainers as well.

Comment #38

karanpagare commented 4 March 2024 at 11:12

Status:

Needs work

» Needs review

Comment #39

smustgrave commented 4 March 2024 at 14:14

Status:

Needs review

» Needs work

Before pinging the sub-maintainer moving to NW for the issue summary update

Comment #40

shweta__sharma commented 19 March 2024 at 07:01

Issue summary:	View changes
Status:	Needs work	» Needs review
Issue tags:	-Needs issue summary update

Updated the screenshots in the IS

Thanks

Comment #41

pemanamgay commented 28 March 2024 at 09:39

Status	File	Size
new	before MR.png	37.24 KB
new	after MR.png	45.3 KB

Here, I have checked MR !6566 and applied it successfully. It has cleanly removed the last row border from the table and the font size remains in pixels after applying the MR from @karanpagare. I'm attaching the screenshot for review

Comment #42

needs-review-queue-bot commented 24 April 2024 at 12:39

Status:

Needs review

» Needs work

Status	File	Size
new	3420865-nr-bot.txt	90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Comment #43

25 April 2024 at 03:43

Gauravvvv opened merge request !7718

Comment #44

gauravvvv commented 25 April 2024 at 03:50

Status:

Needs work

» Needs review

Comment #45

smustgrave commented 28 April 2024 at 15:30

Status:

Needs review

» Needs work

@Gauravvvv there was already a 11.x branch can you update that one please.

Comment #46

7 May 2024 at 06:06

Binoli Lalani made their first commit to this issue’s fork.

Comment #47

binoli lalani commented 7 May 2024 at 06:19

Status:

Needs work

» Needs review

Hello,

I resolved the conflicts of 6566 MR and hide 7718 MR.

Please review.

Thank you!

Comment #48

7 May 2024 at 06:19

Binoli Lalani changed the visibility of the branch 3420865-Olivero-table-head to hidden.

Comment #49

needs-review-queue-bot commented 19 May 2024 at 23:28

Status:

Needs review

» Needs work

Status	File	Size
new	3420865-nr-bot.txt	1.26 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Comment #50

24 May 2024 at 05:39

ahsannazir made their first commit to this issue’s fork.

Comment #51

24 May 2024 at 05:39

Version:

11.x-dev

» main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.