-
Notifications
You must be signed in to change notification settings - Fork 215
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
feat: update filter naming on courseware search modal #1255
feat: update filter naming on courseware search modal #1255
Conversation
Update names to be one of the folow: 1. All Content 2. Text 3. Video 4. Section 5. Other
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1255 +/- ##
=======================================
Coverage 87.97% 87.98%
=======================================
Files 276 276
Lines 4742 4744 +2
Branches 1193 1195 +2
=======================================
+ Hits 4172 4174 +2
Misses 554 554
Partials 16 16 ☔ View full report in Codecov by Sentry. |
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.
Reviewed and have questions
@@ -73,38 +52,6 @@ function renderComponent(props = {}) { | |||
describe('CoursewareSearchResultsFilter', () => { | |||
beforeAll(initializeMockApp); | |||
|
|||
describe('filteredResultsBySelection', () => { |
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.
Why were all these unit tests removed? Wouldn't this lower the code coverage?
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.
Nop, because the filters are now autogenerated based on the results. @rijuma added a mocked factory that returns all different kind of results, and bellow I am checking that we have all the allowed filters:
- All Content
- Text
- Video
- Section
- Other
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.
Here: https://github.com/openedx/frontend-app-learning/pull/1255/files#diff-196671061483745d24904f5c4b39b9505ee34fd28ed336cdf52499eec362b4ceR67 I am checking to have all the filters based on this mock data https://github.com/openedx/frontend-app-learning/blob/master/src/course-home/courseware-search/test-data/mocked-response.json
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.
👍
Update names to be one of the follows:
https://2u-internal.atlassian.net/browse/KBK-79