feat: Add Invite Admins table with header and kebab menu actions#1744
feat: Add Invite Admins table with header and kebab menu actions#1744pbitla-sonata wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1744 +/- ##
==========================================
+ Coverage 87.12% 87.15% +0.02%
==========================================
Files 781 785 +4
Lines 17886 17947 +61
Branches 3748 3738 -10
==========================================
+ Hits 15583 15641 +58
- Misses 2228 2231 +3
Partials 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pbitla-sonata
force-pushed
the
pbitla/ENT-11262
branch
from
eff5c0b to
cd08691
Compare
hkumar1-sonata
left a comment
hkumar1-sonata
left a comment
There was a problem hiding this comment.
Changes Looks Good to me Just added some small suggestions / questions to address.
| </Row> | ||
| </Col> | ||
| <Col> | ||
| <h5 className="pt-2 text-uppercase">Joined org</h5> |
There was a problem hiding this comment.
can we use ii18 for Joined Org?
| const data = camelCaseObject(response.data); | ||
| setEnterpriseAdminsTableData({ | ||
| itemCount: data.count, | ||
| pageCount: data.numPages ?? Math.floor(data.count / options.pageSize), |
There was a problem hiding this comment.
Using Math.floor can undercount pages when the total isn’t a multiple of pageSize, making the last page unreachable.
Suggestion: use Math.ceil(data.count / args.pageSize) to correctly include partial pages and avoid missing records.
pbitla-sonata
force-pushed
the
pbitla/ENT-11262
branch
from
6658352 to
5f667e2
Compare
hkumar1-sonata
left a comment
hkumar1-sonata
left a comment
There was a problem hiding this comment.
Looks Good to me .
kiram15
left a comment
kiram15
left a comment
There was a problem hiding this comment.
I don't think this is powered by the backend yet, and cannot be merged in until it is. When it is ready to go, the PR description should have a screenshot or screen recording of the data table and its functionality locally (adding filters, searching, kebab menu actions). I also don't think its a good idea to include so many tickets in just one PR (creating the data table, adding the kebab menu) because it makes it a lot harder to review and iterate.
| const data = camelCaseObject(response.data); | ||
| setEnterpriseAdminsTableData({ | ||
| itemCount: data.count, | ||
| pageCount: data.numPages ?? Math.ceil(data.count / options.pageSize), |
There was a problem hiding this comment.
Look at other pageCount instances and follow their example
| import { useCallback, useMemo, useState } from 'react'; | ||
| import { camelCaseObject } from '@edx/frontend-platform/utils'; | ||
| import { logError } from '@edx/frontend-platform/logging'; | ||
| import { debounce, snakeCase } from 'lodash-es'; |
There was a problem hiding this comment.
Organize imports into external (react and lodash-es in this case) and then edx packages.
|
|
||
| /* ---------------- MOCK PARAGON ---------------- */ | ||
|
|
||
| jest.mock('@openedx/paragon', () => { |
There was a problem hiding this comment.
It's not helpful to mock paragon in this way, we should directly importing the Card component
| import { useIntl, FormattedMessage } from '@edx/frontend-platform/i18n'; | ||
| import { | ||
| ActionRow, Button, Skeleton, Toast, useToggle, | ||
| Tabs, Tab, |
There was a problem hiding this comment.
Alphabetize imports and keep on one line
| import InviteAdminsTable from './InviteAdminsTable'; | ||
|
|
||
| const PeopleManagementPage = ({ enterpriseId }) => { | ||
| const PeopleManagementPage = ({ enterpriseId, adminsTabEnabled }) => { |
There was a problem hiding this comment.
This will have to have the feature flag code merged in before we attempt this PR, because otherwise where is this bool being passed from?
4 participants
attached screenshot: