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

Refactor of category-select-table component #229

Merged

Conversation

jarmoza
Copy link
Contributor

@jarmoza jarmoza commented Nov 15, 2022

Newly refactored category select table on the categorization page...

  1. has required input (categories, categoryClasses) directly from the store
  2. former instructions and title props have also been removed, relegated to the new refactored categorization page that will act more like a layout.
  3. selectedCategory is still emitted to the categorization page (which maintains a currentCategory field to share between the category-select-table and the column-linking-table)

The component test...

  1. Mocks store required input via getters
  2. Runs through selection of all the default categories.
  3. Spies for the emitted selected category output

@jarmoza jarmoza requested a review from surchs November 15, 2022 20:50
@surchs surchs changed the base branch from master to dev_components_talk_to_store_directly November 16, 2022 04:18
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Hey @jarmoza: really cool PR 🎉 and a very good size of changes and unit of work! This is great!
I checked it out, confirmed it works. I do have two comments:

  1. A minor one about state keeping -> I think whoever holds state for currentCategory, it should only be in one place (see detailed comments inline)
  2. More important: the tests should be as simple as we can, and only concerned with the component. So I'd say: let's get rid of the store methods and unused state variables and just write explicitly what the getters will give to the component at runtime (again, more detail in comments).

components/category-select-table.vue Outdated Show resolved Hide resolved
components/category-select-table.vue Show resolved Hide resolved
cypress/component/category-select-table.cy.js Outdated Show resolved Hide resolved
cypress/component/category-select-table.cy.js Outdated Show resolved Hide resolved
@jarmoza
Copy link
Contributor Author

jarmoza commented Nov 22, 2022

@surchs I believe with these last two commits, we are good to rebase this PR with https://github.com/neurobagel/annotation_tool/tree/dev_components_talk_to_store_directly

Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Awesome, @jarmoza. Yes, fully agreed. This is ready to be squashed into the dev branch! 🎉

@jarmoza jarmoza merged commit 1f1e7c9 into dev_components_talk_to_store_directly Nov 22, 2022
@jarmoza jarmoza deleted the category-table-datadict-refactor branch November 22, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants