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

Correct cell bg color for striped rows #989

Merged
merged 1 commit into from
Dec 23, 2023

Conversation

cpeel
Copy link
Member

@cpeel cpeel commented Dec 22, 2023

Fix setting the has-diff cell background color in Review Work for striped tables.

Task 2067

This is testable in: https://www.pgdp.org/~cpeel/c.branch/fix-review-work-style/

Specifically see srjfoo's diff in the sandbox vs the main TEST code -- you need to be a site admin or PF on TEST to see another user's diffs.

Fix setting the has-diff cell background color in Review Work for
striped tables.

Task 2067
@cpeel cpeel self-assigned this Dec 22, 2023
@cpeel cpeel merged commit cbd2033 into DistributedProofreaders:master Dec 23, 2023
4 checks passed
@cpeel cpeel deleted the fix-review-work-style branch December 23, 2023 19:46
Copy link
Collaborator

@windymilla windymilla left a comment

Choose a reason for hiding this comment

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

Sorry I didn't get to review this yesterday.

Although this works, I was under the impression that !important should be avoided unless there is no alternative. Just looking online now, an example quote is at W3 schools:

Tip: It is good to know about the !important rule. You might see it in some CSS source code. However, do not use it unless you absolutely have to.

I think an alternative (which may have been considered) would be to (re-)specify more precisely than the striped-ness, something like (untested):

td.has-diff {
    background-color: @table-cell-ok-lowc;
}
...
table.theme_striped {
    tr:nth-child(even) {
        background-color: @page-background;
        td.has-diff {
            background-color: @table-cell-ok-lowc;
        }
    }
    tr:nth-child(odd) {
        background-color: @sidebar-background;
        td.has-diff {
            background-color: @table-cell-ok-lowc;
        }
    }
}

@cpeel
Copy link
Member Author

cpeel commented Dec 24, 2023

I think an alternative (which may have been considered) would be to (re-)specify more precisely than the striped-ness, something like (untested):

I did consider that but I didn't like how the table theme CSS then had to "know" about the has-diff -- I wanted to keep them separate. That's why I went with the !important to have the has-diff override whatever else comes its way. It could very well be a poor solution we'll need to undo later but I wanted to start there.

@windymilla
Copy link
Collaborator

OK - I'm happy with that logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants