-
Notifications
You must be signed in to change notification settings - Fork 31
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
Refactor LearnerCreditAllocationTable #1019
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1019 +/- ##
==========================================
- Coverage 82.88% 82.88% -0.01%
==========================================
Files 401 401
Lines 8730 8727 -3
Branches 1801 1800 -1
==========================================
- Hits 7236 7233 -3
Misses 1455 1455
Partials 39 39
☔ View full report in Codecov by Sentry. |
The mocks show that the content title was more bolded or larger than the email, but in your screenshot both the title and email share the same typeface. Was this intentionally deferred? https://2u-internal.atlassian.net/browse/ENT-7431 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, to my vanilla react eye
src/components/learner-credit-management/LearnerCreditAllocationTable.jsx
Outdated
Show resolved
Hide resolved
src/components/learner-credit-management/LearnerCreditAllocationTable.jsx
Show resolved
Hide resolved
b0f55c9
to
2920f84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a nit about disabled ESLint rules.
src/components/learner-credit-management/LearnerCreditAllocationTable.jsx
Outdated
Show resolved
Hide resolved
3e07dfb
to
b7bc888
Compare
b7bc888
to
6c5cd39
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just one non-blocking question.
@@ -63,18 +63,15 @@ const applySortByToOptions = (sortBy, options) => { | |||
}; | |||
|
|||
const applyFiltersToOptions = (filters, options) => { | |||
const userSearchQuery = filters?.find(filter => filter.id === 'userEmail')?.value; | |||
const courseTitleSearchQuery = filters?.find(filter => filter.id === 'courseTitle')?.value; | |||
const courseProductLineSearchQuery = filters?.find(filter => filter.id === 'courseProductLine')?.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up from Slack conversation:
[clarification] in the current state of the (LCM) world, is the courseProductLine filter being used?
const courseProductLineSearchQuery = filters?.find(filter => filter.id === 'courseProductLine')?.value;
[Emily] probably not.
[Adam] even if not used in the current state, since the "Product" column you're keeping in for now isn't exposed in the filters (i.e., disableFilters: true), Markhors next iteration probably will be using it again lolol. So maybe we just leave it in for now with that assumption it will be used again in the near future.
if (courseTitleSearchQuery) { | ||
Object.assign(options, { searchCourse: courseTitleSearchQuery }); | ||
} | ||
const searchQuery = filters?.find(filter => filter.id.toLowerCase() === 'enrollment details')?.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious/clarification] Do you happen to know where this filter id of "enrollment details" (with the space) is coming from? My understanding was that the filter.id
was based on the accessor
for the column (e.g., like courseProductLine
above). But the accessor for the "Enrollment details" column is an object (first time seeing this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was surprised as well. It appears that if the accessor is a function (which in this case, it is), it defaults the filter.id
to the Header
of the column.
Refactor the
LearnerCreditAllocationTable
to have a singleEnrollment Details
column for both the user email or the course title, and also a single input for filtering the search results.This uses the API filtering implementation from here: openedx/edx-enterprise-data#388
Previous table:
Table after changes:
For all changes
Only if submitting a visual change