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

fix: [M3-8533] - Broken firewall rules table #11109

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

pmakode-akamai
Copy link
Contributor

@pmakode-akamai pmakode-akamai commented Oct 16, 2024

Description 📝

The table/list header row in the firewall rules table is not the correct color in dark/light mode. It is hard to tell that it is intended to be a table/list header.

Changes 🔄

  • Changed the Firewall rules table to match our normal tables.
    • Updated the styles to match our normal table colors.
  • Updated PolicyRow
    • Changed it to have borders on all sides.
    • Changed the alignment of the Select and helper text to be aligned with the Action column for screens < lg, and with the last column for screens >= lg.

Target release date 🗓️

N/A

Preview 📷

Breakpoints Before After
lg Screenshot 2024-10-16 at 1 31 23 PM Screenshot 2024-10-16 at 1 35 28 PM Screenshot 2024-10-16 at 1 32 19 PM Screenshot 2024-10-16 at 1 34 43 PM
between sm and lg Screenshot 2024-10-16 at 1 38 54 PM Screenshot 2024-10-16 at 1 37 30 PM
sm Screenshot 2024-10-16 at 2 19 32 PM Screenshot 2024-10-16 at 7 29 38 PM

How to test 🧪

Reproduction steps

(How to reproduce the issue, if applicable)

Verification steps

  • Ensure the table is not broken in both dark and light modes.
  • Verify table across different screen sizes and ensure it matches our normal table colors.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@pmakode-akamai pmakode-akamai self-assigned this Oct 16, 2024
@pmakode-akamai pmakode-akamai changed the title Fix: [M3-8533] - Broken firewall rules table fix: [M3-8533] - Broken firewall rules table Oct 16, 2024
@pmakode-akamai pmakode-akamai marked this pull request as ready for review October 16, 2024 09:26
@pmakode-akamai pmakode-akamai requested a review from a team as a code owner October 16, 2024 09:26
@pmakode-akamai pmakode-akamai requested review from bnussman-akamai and abailly-akamai and removed request for a team October 16, 2024 09:26
Copy link

github-actions bot commented Oct 16, 2024

Coverage Report:
Base Coverage: 87.06%
Current Coverage: 87.06%

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update the padding of the cells to match our other tables? They have 15px left and right. Header cells have an additional 10px top and bottom.

Other tables FirewallRuleTable
Header cells
Screenshot 2024-10-16 at 6 04 27 PM Screenshot 2024-10-16 at 6 04 08 PM
Body cells
Screenshot 2024-10-16 at 6 09 09 PM Screenshot 2024-10-16 at 6 09 52 PM

Copy link
Contributor Author

@pmakode-akamai pmakode-akamai Oct 17, 2024

Choose a reason for hiding this comment

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

Good catch! 👍 I will update it.

edit: Done ✅

@pmakode-akamai pmakode-akamai marked this pull request as draft October 17, 2024 08:15
@pmakode-akamai pmakode-akamai marked this pull request as ready for review October 17, 2024 12:42
@pmakode-akamai pmakode-akamai force-pushed the M3-8533-fix-broken-firewall-rules-table branch from 88dbf78 to dd3015e Compare October 17, 2024 12:51
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks much better!

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Such styling discrepancy usually points at markup or composition problems.

In this case, the <FirewallRuleTable > is very much a misnomer considering it renders div grid in place an actual HTML table. I am not why it was built that way (perhaps to account for the draggable aspect of its rows?) but I'd like explore implementing the right semantic markup instead of putting a styling band-aid. There would benefits doing so, namely consistency with our other features, styling and accessibility, as well as making sure this will update when we update any table related styling.

It increases the scope fo this particular PR, but worth exploring.

@pmakode-akamai
Copy link
Contributor Author

pmakode-akamai commented Oct 17, 2024

Such styling discrepancy usually points at markup or composition problems.

In this case, the <FirewallRuleTable > is very much a misnomer considering it renders div grid in place an actual HTML table. I am not why it was built that way (perhaps to account for the draggable aspect of its rows?) but I'd like explore implementing the right semantic markup instead of putting a styling band-aid. There would benefits doing so, namely consistency with our other features, styling and accessibility, as well as making sure this will update when we update any table related styling.

It increases the scope for this particular PR, but worth exploring.

I agree that moving to more semantic markup for the <FirewallRuleTable > would enhance accessibility and consistency. However, I believe this change is beyond the scope of the current PR/ticket.

I suggest we have a separate ticket to explore this implementation in detail. This way, we can ensure it receives the attention it deserves without delaying the current changes. Let me know what you think about this.

@pmakode-akamai
Copy link
Contributor Author

@abailly-akamai I’ll explore this further and see how we can approach it effectively. Thanks!

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

Successfully merging this pull request may close these issues.

4 participants