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

Common Table component (part 1) #2376

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

lindapaiste
Copy link
Collaborator

Fixes #1442
Fixes #2375
Progress on #2358

This is part 1 because I have cleaned up the __Table components, but not the __TableRow components.

Code cleanup:

  • Create a TableBase component to handle common logic of tables:
    • Show <Loader/> when loading.
    • Show "no results" message when empty.
    • Render column headers with the name of each column.
    • Highlight the active column with underline and triangle up/down icon.
    • Sort the items in the table when clicking on the column header.
  • Write tests for this core logic of the TableBase and TableHeaderCell.
  • Use the TableBase in existing components:
    • AssetList - converted to function component
    • CollectionList - converted to function component
    • SketchList - converted to function component
    • Collection - still a class component because I have other open PRs that touch this (Move CollectionMetadata into its own component. #2253)
  • Sorting is handled internally by the TableBase, so remove sorting from Redux.
    • Delete the sorting reducer, actions, and selectors.
    • Rework one test of SketchList which checked redux actions.
  • Use class sketches-table on all tables (can add additional classes alongside it), and delete SCSS properties on asset-table which are duplicates.
  • Migrate to styled-components:
    • All table header styles.
    • Empty message style.

Bug Fixes:

New Features:

  • AssetList can be sorted.
  • Added ARIA attributes aria-sort and aria-pressed to column headers.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

# Conflicts:
#	client/modules/User/components/Collection.jsx
#	client/styles/components/_sketch-list.scss
@lindapaiste
Copy link
Collaborator Author

This PR has some non-trivial conflicts with the new responsive design. Specifically, storing the sorting state within the table component presents a problem with the sort dropdown menu that is outside of the table. I probably need to keep the sorting state in Redux.
image

# Conflicts:
#	client/modules/IDE/components/CollectionList/CollectionList.jsx
#	client/modules/IDE/selectors/collections.js
#	client/modules/IDE/selectors/projects.js
#	client/modules/User/components/Collection.jsx
@lindapaiste
Copy link
Collaborator Author

Not quite as 🧹 as before but I fixed the conflicts by putting the sorting state back into Redux.

# Conflicts:
#	client/modules/IDE/components/AssetList.jsx
#	client/modules/IDE/components/CollectionList/CollectionListRow.jsx
#	client/modules/IDE/components/SketchList.jsx
#	client/modules/IDE/components/__snapshots__/SketchList.unit.test.jsx.snap
#	client/styles/components/_sketch-list.scss
@lindapaiste
Copy link
Collaborator Author

After all the merges there is something a bit wonky about the sorting, where sometimes clicking on the table header flips the sorting twice. This is reflected in the failing test. I'm investigating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: PRs to Review
Development

Successfully merging this pull request may close these issues.

Sketches table on collection page is not sortable Create generic Table component
2 participants