change: [M3-8533, M3-8761] - Fix firewall rules table and Replace react-beautiful-dnd with dnd-kit lib

[pmakode-akamai]

Description 📝

While working on ticket M3-8533 & PR #11109, we decided to implement/use semantic markup for the Firewall rules table to improve consistency and accessibility, rather than applying a styling band-aid to div grids to make it look like our normal tables. The react-beautiful-dnd library has made us rely on div(Box) grids instead of our normal tables, which complicates using semantic tables/grid layouts because of the draggable elements. Plus, this library has been deprecated (announcement: atlassian/react-beautiful-dnd#2672).

To solve these problems and make better use of our normal tables, switching to dnd-kit(https://dndkit.com/) would be the great idea, which is more popular, supports grid layouts better, and can replace react-beautiful-dnd. This will help us use our semantic tables properly and fix the issues with the Firewall rules table in both dark and light modes.

Changes 🔄

List any change relevant to the reviewer.

• Used Normal tables instead of Div Box grids
• 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 < 'md', and with the last column for screens >= 'md'.
• Replaced react-beautiful-dnd with dnd-kit lib
• Fix <Table /> styling for noOverflow property

Target release date 🗓️

N/A

Preview 📷

Before	After

How to test 🧪

Prerequisites

• Clear cache or test in Incognito.

Reproduction steps

• Go to any Firewall details page (https://cloud.linode.com/firewalls/948204/rules)
• Observe the broken looking table (in both dark and light mode)

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.
• Ensure the drag and drop functionality work as expected in different screen sizes.
• Verify all firewall rule actions work as expected.

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

[github-actions]

Coverage Report: ❌
Base Coverage: 87.32%
Current Coverage: 86.95%

[abailly-akamai]

Fantastic work @pmakode-akamai, and really appreciate considering going with a more holistic refactor, including replacing a non-supported library.

Implementation, markup and styling looks great (in both modes) for the most part, leaving some comments that shouldn't take too much extra work to implement.

[abailly-akamai]

@pmakode-akamai I believe the e2e failure is relevant here - you may want to take a look at firewalls/update-firewall.spec.ts

[bnussman-akamai]

Thanks for working on this!

For whatever reason, the dragging is not nearly as smooth as the previous implementation for me. I did try the dndkit examples and they were much smoother. Could this be something wrong on our end?

develop	This PR
Screen.Recording.2024-10-22.at.10.44.04.AM.mov	Screen.Recording.2024-10-22.at.10.38.40.AM.mov

I'm also noticing that the table row is can't overflow outside of the table like it did previously when dragging. Not a blocker, but I liked how previously the row was "on top" of everything when dragging.

[pmakode-akamai]

For whatever reason, the dragging is not nearly as smooth as the previous implementation for me. I did try the dndkit examples and they were much smoother. Could this be something wrong on our end?

Hi @bnussman-akamai, I think we can definitely add a transition (something like in this) to make the drag animation smoother. However, I'm having some difficulty figuring out how the dnd-kit is handling onDragEnd in our case.

I'm encountering a similar issue to what was mentioned here when I try to add the transition. Looks like dnd-kit works fine with local state but not so much with async state (as per this comment). In our case, the way we are reordering onDragEnd is not straight forward.

I will check how we can add a transition in our case somehow.

I'm also noticing that the table row is can't overflow outside of the table like it did previously when dragging. Not a blocker, but I liked how previously the row was "on top" of everything when dragging.

I agree, the DndContext is currently applied to the table, which is why the table row can’t overflow outside of it like it did previously. We can explore adjustments to allow the row to appear 'on top' during dragging.

[pmakode-akamai]

@abailly-akamai @bnussman-akamai

For whatever reason, the dragging is not nearly as smooth as the previous implementation for me. I did try the dndkit examples and they were much smoother. Could this be something wrong on our end?

update: The smoothness while dragging rows has been improved! ✅

I'm also noticing that the table row is can't overflow outside of the table like it did previously when dragging. Not a blocker, but I liked how previously the row was "on top" of everything when dragging.

To allow dragging over the top of current page / overflow outside of the table, we can simply add a noOverflow={true} prop to the <Table /> component (as overflow is not allowing us to drag over the top). However, this changes the styling of the header due to how noOverflow is implemented in our <Table /> component.

• With noOverflow={true}
  

Screen.Recording.2024-10-23.at.10.37.43.PM.mov

I’d like to confirm if we want to apply this header styling with noOverflow={true} for the FirewallRulesTable. Additionally, should we maintain the overflow settings for Firewall Rules Table, considering that the version in production does not have overflow?

[abailly-akamai]

@pmakode-akamai @bnussman-akamai

It struck me that adding noOverflow to allow overflow was smelly 😄

I pushed a commit to:

• Make sure the table header styling isn't dependent on the noOverflow property (this was clearly a bug)
• Reverted the styling so noOverflow actually implements the proper CSS
  I looked at a couple components that had this property declared and did not see any regression. Not sure what the intent was when adding it, but since it did the opposite it's worth checking

[abailly-akamai]

Great work - thanks for the contribution 👍

[bnussman-akamai]

In production I'm able to re-order rules with the keyboard (using space then up and down arrow keys). In this PR I don't think I can do that, but I may be missing something. Is this something we can accomplish with dndkit?

[abailly-akamai]

@bnussman-akamai

The team seems to have put great care into accessibility for this component, which is great - https://docs.dndkit.com/guides/accessibility#keyboard-support

This implementation was missing the keyboard sensors, which surprise me isn't a default. Anyway, added them. You experience may differ slightly from the previous plugin tho

[hkhalil-akamai]

I don't think the KeyboardSensor achieves the effect we're looking for. It moves items in the keyed direction by 25px, which can make it difficult to precisely change the place of the item. It allows moving the item left and right and also scrolls the page.

Screen.Recording.2024-10-23.at.6.47.21.PM.mov

I can't find an option within dnd-kit that accomplishes the prior behavior. My recommendation is to setup a custom key handler that calls triggerReorder as well as e.preventDefault() to prevent the scrolling issue.

[abailly-akamai]

That's fair!

I can't find an option within dnd-kit that accomplishes the prior behavior.

I believe the commit underneath achieves parity with the previous behavior, while still using the kit and avoiding a custom key handler. Can you confirm?

680049a

[hkhalil-akamai]

This gets closer but I'm still seeing some weird behavior when the page has vertical scrolling enabled. It's hard to describe but you can see it for yourself by Ctrl-+'ing in until the page overflows.

[pmakode-akamai]

@abailly-akamai @hkhalil-akamai I tried different ways and even used e.preventDefault() to prevent window scrolling (when the browser has vertical scroll enabled) while the KeyboardSensor is active. However, this didn't work due to how the dnd-kit KeyboardSensor is implemented, as it adds default scrolling behavior. After extensive digging 🥲, I found that the handleKeyDown method from the KeyboardSensor does not allow us to override the scrolling behavior when using keyboard keys. As a result, I customized the dnd-kit library code itself to meet our requirements 😅, and some how I was able to prevent the scroll behavior when using keyboard keys ✅

Could you take a look?

a839fef

[hkhalil-akamai]

Great digging, this works exactly like I would expect it to! 👏🏽🎉

[pmakode-akamai]

@abailly-akamai

• I looked at a couple components that had this property declared and did not see any regression. Not sure what the intent was when adding it, but since it did the opposite it's worth checking

Thank you for the changes, but I wonder if the intent was to enable a horizontal scrollbar for all tables by default, without the noOverflow property (for smaller screens or at time when the table content tend to overflow)?

Browser window adjusted / or on smaller screens:

On Prod	On this Branch with these changes

If the plan is to apply overflowX: auto to all our normal tables by default (incase), we need to ensure this doesn't affect the Firewall Rules Table. As, we want to allow row dragging over the top in this case, which won’t be possible if overflowX: auto is enabled. With some changes here; we may need to add noOverflow property specific to the Firewalls Rules table.

[abailly-akamai]

Yeah sorry I am working too fast on this one - I'll revert that part but keep the styling fix

[hkhalil-akamai]

Great work @pmakode-akamai @abailly-akamai! 🎉

[pmakode-akamai]

@bnussman-akamai Are we good to merge this PR? thanks!!

[abailly-akamai]

@pmakode-akamai have you confirmed the failing tests are unrelated?

[pmakode-akamai]

@pmakode-akamai have you confirmed the failing tests are unrelated?

@abailly-akamai yea, this should be unrelated

confirming all checks for these 2 specs pass locally. I think all checks should pass on re-run.

[linode-gh-bot]

Cloud Manager UI test results

🎉 445 passing tests on test run #32 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
0 Failing	445 Passing	2 Skipped	81m 30s