Skip to content
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

Retrieve column values from state object #441

Merged
merged 9 commits into from
Nov 15, 2021

Conversation

rmainwork
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality
    to change)

Checklist:

  • I have run eslint on the code
  • I have added JSDoc for all of my code (where applicable)
  • I have added tests to cover my changes.

Priority:

  • Normal
  • High

Related Issues:

Fixes #437

Refactor course instance table to obtain the default list of columns
from a state value rather than a hard-coded list of columns
@rmainwork rmainwork changed the base branch from main to develop November 12, 2021 20:45
Copy link
Contributor

@jonseitz jonseitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly small stylistic things here, plus a suggestion for fixing the broken tests...

src/client/components/pages/Courses/CoursesPage.tsx Outdated Show resolved Hide resolved
src/client/components/pages/Courses/CoursesPage.tsx Outdated Show resolved Hide resolved
src/client/components/pages/Courses/CoursesPage.tsx Outdated Show resolved Hide resolved
src/client/components/pages/Courses/CoursesPage.tsx Outdated Show resolved Hide resolved
src/client/components/pages/Courses/CoursesPage.tsx Outdated Show resolved Hide resolved
Robert Main added 7 commits November 15, 2021 10:53
The lack of a "meetings" column in the default view meant that the
meeting modal integration tests were failing as they were expecting an
"Edit Meeting" button to be visible. Since the meetings column was not
visible, this was not the case and the tests thusly failed.

This commit adds the meetings column into the default view to fix these
breaking tests
Going by the previous course planner, `notes` is an optional column
Since this expression just returns an array, we don't need the spread operator
here
Other pages (such as `FacultyAdmin` and `CourseAdmin`) all use the
`VerticalSpace` component for providing vertical space around the page
content
Aside from being deprecated in later versions(
testing-library/dom-testing-library#416) it's also
not needed here, as we can just return `findByDisplayValue` directly
The type can automatically be inferred from `defaultView` and usage of
`T as Type` can actually interfere with other type checking done by
TypeScript.
@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #441 (57b800d) into develop (a72e55f) will decrease coverage by 0.01%.
The diff coverage is 93.93%.

❗ Current head 57b800d differs from pull request most recent head 051ae02. Consider uploading reports for the commit 051ae02 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #441      +/-   ##
===========================================
- Coverage    97.82%   97.80%   -0.02%     
===========================================
  Files          151      152       +1     
  Lines         2761     2785      +24     
  Branches       278      278              
===========================================
+ Hits          2701     2724      +23     
- Misses          26       27       +1     
  Partials        34       34              
Impacted Files Coverage Δ
src/client/components/App.tsx 100.00% <ø> (ø)
...rc/client/components/pages/Courses/CoursesPage.tsx 93.02% <84.61%> (-1.27%) ⬇️
src/client/components/layout/Message.tsx 100.00% <100.00%> (ø)
src/client/components/pages/CourseAdmin.tsx 100.00% <100.00%> (ø)
...t/components/pages/Faculty/FacultyAbsenceModal.tsx 97.56% <100.00%> (+0.26%) ⬆️
src/client/components/pages/FacultyAdmin.tsx 100.00% <100.00%> (ø)
...t/components/pages/utils/messageHelperFunctions.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e7853b...051ae02. Read the comment docs.

Copy link
Contributor

@jonseitz jonseitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just responded to the note about updating the end-to-end test in the future, but this is all set. 👍

@rmainwork rmainwork merged commit a4ffa15 into develop Nov 15, 2021
@rmainwork rmainwork deleted the feature/437-course-instance-default-view branch November 15, 2021 18:04
@jonseitz jonseitz mentioned this pull request Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Course instance table: Displaying default view of columns
2 participants