-
Notifications
You must be signed in to change notification settings - Fork 17
refactor: improve OrchestratorList maintainability and mobile layout (Fixes #466) #486
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
base: main
Are you sure you want to change the base?
Conversation
|
@Roaring30s is attempting to deploy a commit to the Livepeer Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
@ECWireless Feel free to let me know if you are fine with this card design and my approach to the refactors. If all is good, we can employ a similar card approach and refactor to the rest of the tables in explorer that also need mobile responsiveness. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@Roaring30s this is awesome! And really appreciate the level of thought you put into the PR write-up. I think solution 3 is the best route for sorting, to be honest; though I recognize that this could be a lot of work, so want to be realistic about what you're willing to contribute with this. Please let me know if you need any kind of support! |
|
@ECWireless Sounds good. I'll implement it. Just wanted to get your guys' opinion if you were fine with me implementing solution 3 in the PR. And thank you for checking my code and giving me feedback. These PR's are helping me learn. Ill have this done today as well. |
|
@Roaring30s The refactors into reusable components and helper functions could make sense if these components are already shared elsewhere, otherwise, it may be better to keep them local for now. @ECWireless would you mind taking a look? If it makes sense, we can also move those refactors into a separate PR. |
|
@rickstaa I can investigate this. But when looking at the live website, I am not seeing some body-level horizontal scroll activating. Only at the orchestrator table level.
|
Your correct, sorry I was mixing up two issues l. The problem is on the orchestrators page. The problem is that the Screen_Recording_20260113_152039_Brave.mp4 |
Okay cool thanks for the vid. Ill fix this. One last thing. While EC reviews it, even if some of the components only have a one time use, the refactors I made will make it very easy for us to test things in isolation like the PaginationControls and putting things in hooks allows me to more easily set boundaries in testing. Which was something I was going to eventually throw my hat in the ring with was to help expand test coverage for the repo. So that was where my thinking was going. We can nix the mobile compatibility code I have in this PR but still preserve the refactors for the purpose mentioned above but you guys let me know. |
|
@Roaring30s looks like this is hitting linting/type errors |
Yeah. I fixed a conflict this morning and this came up so Ill clean it up. |
|
@ECWireless Lint errors fixed. Note. This is not ready for merge but just some quick feedback is needed from your end.
You let me know what approach works best for your team and Ill accommodate. With the feedback, Ill get the PR merge ready. |


Refactor: OrchestratorList Component Improvements
Overview
This PR refactors the
OrchestratorListcomponent to improve maintainability, readability, and mobile user experience. The changes include component extraction, logic centralization, and responsive design improvements.Fixes Issue #466
Refactoring for Maintainability & Readability
Component Extraction
To improve testability and maintainability, several components were extracted from the monolithic
OrchestratorList:OrchestratorCard- Encapsulates the mobile card view UI and logicOrchestratorActionsMenu- Reusable "three dots" action menu (used in both desktop and mobile views)YieldAssumptionsControls- Filter controls UI for forecasted yield assumptionsPaginationControls- Extracted pagination UI into a reusable component within the Table folderLogic Centralization
useOrchestratorViewModel- Custom hook that centralizes:mappedData)useBondingManagerAddress,useReadContract)useOrchestratorRowViewModel- Per-row formatting hook for:feeCut,rewardCut,rewardCallscalculationsUtility Function Consolidation
formatTimeHorizonandformatFactorstolib/roi.tsto centralize ROI-related utilitiesUnified Rendering Approach
renderCardprop pattern with conditional column arrays (desktopColumnsvsmobileColumns)Tablecomponent with different column configurationsconstrainWidthprop toTablecomponent to handle mobile overflow issues without affecting other tablesMobile Layout Improvements
box-sizing📋 Feedback Request: Mobile Card Design Approach
This section is not for approval, but to gather team feedback.
Since
react-tablev7 is a headless library with no built-in mobile/responsive functionality, I implemented a custom card-based layout for mobile devices. This approach:Tablecomponent infrastructureQuestion for the team: Are we comfortable with this card-based approach being the default pattern for mobile table views? This PR introduces several opinionated architectural decisions, and I'd like to ensure the team is aligned before proceeding further.
One last mention: Feel free to let me know if any of my patterns or approaches here are good to keep or some changes are needed. I am doing my best to level myself up so any feedback and criticisms would be heavily appreciated.
🔍 Known Limitation: Mobile Column Filtering
Currently, the mobile card view uses a single column (
earnings) in themobileColumnsarray. This means:The Issue:
Since react-table's sorting is tied to column definitions, and we're only defining one column for mobile, only that column's sorting is available.
Potential Solutions:
mobileColumns- AddtotalStakeandninetyDayVolumeETHcolumns with emptyCellrenderers, keeping them in the array for sorting purposes while only visually rendering the cardTablecomponent with a context provider that exposestoggleSortByandsortBy, allowing custom header components to access sorting functionalityDiscussion Needed:
To add filtering capabilities for the other two columns on mobile, we would need to make changes to the
Tablecomponent (like adding a context provider). Before implementing this, I'd like to discuss:Files Changed
components/OrchestratorList/index.tsx- Main refactoringcomponents/OrchestratorList/OrchestratorCard.tsx- New componentcomponents/OrchestratorList/OrchestratorActionsMenu.tsx- New componentcomponents/OrchestratorList/YieldAssumptionsControls.tsx- New componentcomponents/Table/index.tsx- AddedconstrainWidthprop and contextcomponents/Table/PaginationControls.tsx- New componenthooks/useOrchestratorViewModel.ts- Centralized view model logichooks/useOrchestratorRowViewModel.ts- Per-row formatting hooklib/roi.ts- Added utility functionsTesting
Upon Feedback
If we can come to a consensus on how to approach the Orchestrator Table, then we could easily use this approach to make the other tables in the explorer mobile-compatible as well in one shot.