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 strikethrough to deleted tables #1325

Merged

Conversation

HollaG
Copy link

@HollaG HollaG commented Feb 11, 2025

Add strikethrough to all text contents in
deleted tables. Note that we only target the
non-header rows with the CSS.

Summary:

Fixes part of #1324

Changes Made:

Add CSS style text-decoration: line-through to tables which require a strikethrough

image

Proposed Commit Message:

Make it easier to see which issues are deleted

Allow for further differentiation between issues
that the user wants and doesn't want by 
striking-through all text in the table.

This helps to give a visual difference between the
un-deleted issues, which should prevent confusion 
when the table headers are not visible.

Styles are applied via CSS override when referencing 
the component in CSS, using ::ng-deep.

Add strikethrough to all text contents in
deleted tables. Note that we only target the
non-header rows with the CSS.
@HollaG HollaG requested a review from NorbertLoh February 11, 2025 08:20
Copy link
Contributor

@NorbertLoh NorbertLoh left a comment

Choose a reason for hiding this comment

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

Very clean solution 👍 . Perhaps the note in git commit message can be in a new line as writing it in one line exceeds the hard limit of 72 characters for the subject line. More information for the git commit message guidelines can be found here.

Copy link
Contributor

@Arif-Khalid Arif-Khalid left a comment

Choose a reason for hiding this comment

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

Excellent PR that makes a clear distinction between deleted and non-deleted bugs.
Great use of the cascading property of css. Good job! 😄

@Arif-Khalid
Copy link
Contributor

Arif-Khalid commented Feb 11, 2025

Very clean solution 👍 . Perhaps the note in git commit message can be in a new line as writing it in one line exceeds the hard limit of 72 characters for the subject line. More information for the git commit message guidelines can be found here.

@HollaG I agree with this comment about the commit message and to add on I believe it would also be appropriate to include the reasoning for the change in a similar format as the resource linked.
Additionally, while I think the change is sufficient, we should wait for an update from @damithc on #1324 for additional differentiation before merging.

Added a red border and removed
the drop shadow from the tables
@HollaG
Copy link
Author

HollaG commented Feb 15, 2025

I've updated to code to include a red border as well.

This is up for discussion - how do we code for accessibility, or do we not need to, as there is no border in the other tables so there is still a visual difference?

@damithc
Copy link
Contributor

damithc commented Feb 15, 2025

@HollaG the red border makes it more eye-catching, the opposite of what we want to achieve?
We want to differentiate the deleted issues (so that they are not mistaken for open issues) while making them less eye-catching as possible.

@HollaG
Copy link
Author

HollaG commented Feb 15, 2025

Noted @damithc , I'll remove them

@HollaG
Copy link
Author

HollaG commented Feb 15, 2025

@damithc , on further thought, it might look a little weird with the table not having any borders.

Would this border color be OK? Same color as the row dividers.

image

(Previous design):
image

@damithc
Copy link
Contributor

damithc commented Feb 16, 2025

@damithc , on further thought, it might look a little weird with the table not having any borders.

Would this border color be OK? Same color as the row dividers.

While removing the drop shadow is worth a try, I think best to put it back. Not having it hurts the high-level consistency of the page.
For now, let's use striking off text as the only differentiator between the two tables.

@Arif-Khalid
Copy link
Contributor

Agreed. @HollaG anything more to suggest or are you happy with the current implementation as well?

@HollaG
Copy link
Author

HollaG commented Feb 16, 2025

I will put back the drop shadow. I don't think there are any further changes to be made.

@HollaG HollaG merged commit 5cd955d into CATcher-org:feature-bug-trimming Feb 17, 2025
2 of 3 checks passed
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