Migrate graders_manager.jsx to React Table V8#8002
Conversation
…Updated filter.jsx to account for new component
… migrate-GradersManager
mrafie1
changed the title
Coverage Report for CI Build 27581324586Coverage decreased (-0.05%) to 90.162%Details
Uncovered Changes
Coverage Regressions4 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
@mrafie1 nice work, I left a few inline comments. Also, please make sure to convert the criteria table in this view as well.
You don't need to worry about test coverage for textFilter.
| value={filterValue?.toString() || ""} | ||
| style={{width: "100%"}} | ||
| aria-label={defaultSearchPlaceholderText()} | ||
| aria-label={`${I18n.t("search")} ${column.columnDef.header || ""}`} |
There was a problem hiding this comment.
Keep defaultSearchPlaceholderText, but add an optional parameter to that function to allow us to pass in text that should be appended to the "Search" text. You can then call defaultSearchPlaceholderText(column.columnDef.header) to generate the correct label here.
| enableSorting: true, | ||
| minSize: 90, | ||
| }), | ||
|
|
| accessor: "criteria", | ||
| filterable: false, | ||
| Cell: ({value}) => { | ||
| enableSorting: true, |
There was a problem hiding this comment.
It looks like the sorting isn't working for this column, please look into this
| enableSorting: true, | ||
| minSize: 170, | ||
| }), | ||
|
|
| return {filtered}; | ||
| } | ||
| resetSelection = () => { | ||
| this.state.rowSelection = {}; |
There was a problem hiding this comment.
React state should never be modified directly. Instead, use setState
| getSelectedRows = () => { | ||
| return Object.keys(this.state.rowSelection).map(id => Number(id)); | ||
| }; | ||
| componentDidUpdate(prevProps, prevState, snapshot) { |
There was a problem hiding this comment.
Insert a blank line in between these two functions
| filtered: [], | ||
| columns: this.getColumns(props.showSections, props.sections, props.showCoverage), | ||
| columnFilters: [{id: "inactive", value: false}], | ||
| columns: this.getColumns(), |
There was a problem hiding this comment.
To resolve the question you asked about the showSections and showCoverage props, instead of using them to toggle column visibility in the initialState prop, you can use them as arguments to this.getColumns to control whether those columns exist at all. That is, modify the return value of this.getColumns to only include the section/coverage columns when the corresponding props are true.
|
|
||
| export const defaultSearchPlaceholderText = () => I18n.t("table.search"); | ||
|
|
||
| export default function CaseSensitiveSearchFilter({column, filterValue}) { |
There was a problem hiding this comment.
It's fine to duplicate the caseSensitiveTextFilter component for React table v6, but you need to fully duplicate the logic. In particular, there is no need to store "case sensitive" state in the parent component. Instead, column.setFilterValue should pass an object (keys filterValue, caseSensitive) which are then used by the corresponding filter. This plays the same role as the onChange handler in the existing component.
For more information see #7938.
3 participants
Proposed Changes
(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)
tablefolder to include case sensitive filtering along side searching:case_sensitive_search_filterfilter.jsxto allow this component to be displayed when filterVariant: "case-sensitive-text" is usedsearch_filter.jsxaria-label definitionI18n.t("table.search")column.columnDef.headerfull_namecolumn usessearch_filter.jsxfor its search bar component. However, previous tests forgraders_manager.tsxexpected a search bar with the qualities:{name: ${I18n.t("search")} Name. With the previous definition, only the aria-label${I18n.t("search")}appears when runningscreen.debug(). Mimicking the previous implementation forcaseSensitiveTextFilterintable_helpers.jsx, allows all frontend tests to run and fixes the missing information in the aria-label.getSelectedRows()andresetSelection()<Table>isCaseSensitiveattribute to statetoggleCaseSensitivityfunction togroup_namecolumn's meta-section to allow the filter component to update caseSensitivity.Screenshots of your changes (if applicable)
BeforeAfter
Associated documentation repository pull request (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
(Include any questions or comments you have regarding your changes.)
Small Note:
filter.jsxinsteadVery important question:
<Table>componentinitialStatedefinition means that they cannot change, even if the props being passed in are changing (Correct me if I'm wrong).Please feel free to ask about a specific component. I ended up removing many of the previous functions to make my implementation more similar to Lizzie's and to 'mock' the functionality of the previous implementation.
Am I good to start migrating the Annotations table? I'm kind of interested in pursuing another part of the codebase/doing something different. I'll let you know if I have anything specific in mind.