Removed Table and StatusAlert deprecation of Paragon #899
Conversation
… into abdullahwaheed/remove-paragon-table-deprecation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #899 +/- ##
==========================================
+ Coverage 83.13% 83.33% +0.19%
==========================================
Files 399 398 -1
Lines 8653 8628 -25
Branches 1790 1780 -10
==========================================
- Hits 7194 7190 -4
+ Misses 1421 1400 -21
Partials 38 38
☔ View full report in Codecov by Sentry. |
|
@abdullahwaheed, looks like this needs a rebase. @adamstankiewicz, you might want to take a look here as well. |
… into abdullah/paragon-form-deprecations
… into abdullahwaheed/remove-paragon-table-deprecation
…hub.com:openedx/frontend-app-admin-portal into abdullahwaheed/remove-paragon-table-deprecation
|
@adamstankiewicz, another enterprisey one we need eyes on. |
| variant="danger" | ||
| icon={Error} | ||
| > | ||
| {`${ERROR_MESSAGE} ${error.message}`} |
There was a problem hiding this comment.
nit: Render the alert message within a <p>, e.g.:
<p>Try refreshing your screen ({this.props.error.message})</p>| }); | ||
| sortTable(ordering); |
There was a problem hiding this comment.
Ideally, our tables throughout the app would no longer need to dispatch Redux actions to do pagination/sorting/filtering, etc. but instead use what DataTable offers you via fetchData directly.
| ); | ||
|
|
||
| const renderLoadingMessage = () => ( | ||
| <TableLoadingSkeleton /> |
There was a problem hiding this comment.
We'd ideally rely on DataTable's isLoading prop rather than defining our own loading state for the table at this point.
| className="table-sm table-striped" | ||
| columns={columns} | ||
| data={formatData(data)} | ||
| isSortable={tableSortable} |
There was a problem hiding this comment.
Given that some tables support row selection (e.g., code management), I'm not seeing anything regarding the isSelectable prop here, which implies the selection logic is custom outside of DataTable; ideally, we'd rely on DataTable's selection logic rather than defining our own.
| import { updateUrl } from '../../utils'; | ||
|
|
||
| class TableComponent extends React.Component { |
There was a problem hiding this comment.
Long term, I'm not entirely sure the benefit of keeping TableComponent/TableContainer now that DataTable exists and am considering whether this refactor should move away from these components in favor of implementing DataTable directly where needed.
That way, we don't need to refactor/maintain a complex, single table that supports every single possible table use case throughout the application vs. just implementing the bits and pieces of DataTable as needed for each individual use case instead.
| {!loading && !error && data && data.length === 0 | ||
| && renderEmptyDataMessage()} |
There was a problem hiding this comment.
Should we instead rely on DataTable.EmptyTable for the empty state messaging?
| @@ -241,7 +241,7 @@ class CouponDetails extends React.Component { | |||
|
|
|||
| getNewColumns(selectedToggle) { | |||
| const selectColumn = { | |||
| label: ( | |||
| Header: ( | |||
| <CheckBox | |||
There was a problem hiding this comment.
Checkbox is deprecated in favor of Form.Checkbox
| useEffect(() => { | ||
| // Get initial data | ||
| paginateTable(); | ||
| return clearTable; | ||
| }, []); |
There was a problem hiding this comment.
DataTable calls fetchData on initial component mount to fetch any initial data. We shouldn't need a useEffect to trigger fetching initial data here.
| return clearTable; | ||
| }, []); | ||
|
|
||
| const handlePageChange = (page) => { |
There was a problem hiding this comment.
As mentioned else, pagination changes for DataTable should also come through the fetchData callback function rather than our existing custom implementation.
|
@abdullahwaheed, I think there are still un-addressed comments in this one, right? |
jmbowman
added
waiting on author
… into abdullahwaheed/remove-paragon-table-deprecation
|
this PR is outdated and there are a lot of merge conflicts. closing this one and create a new PR |
Ticket
Migrate off deprecated Paragon components
What has changed
Removed deprecated
Tablecomponent of paragon and updated it withDataTableRemoved deprecated
StatusAlertcomponent of paragon and updated it withAlertReferences
Paragon Table
Paragon DataTable
Paragon Alert
Paragon StatusAlert
Based on #798