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

Updates to Index page for data table + data dictionary for store refactor #329

Merged
merged 12 commits into from
Jan 30, 2023

Conversation

jarmoza
Copy link
Contributor

@jarmoza jarmoza commented Jan 26, 2023

This closes #314 and #320

Several updates here are made for the index page relating to the recent store refactor.

  1. The workflow for the user has changed in that they are no longer able to select a data dictionary without first having selected a data table. This has been accomplished by adding a new feature to file-selector care of the prop enabled. File selection for a data dictionary is disabled until a data table has been selected.
  2. textarea elements on the index page have been converted to one way binding (user input does not edit underlying value) via the value attribute instead of the two-way bind via v-model
  3. Reactivity for the textarea for the data dictionary input has been fixed by reassigning the data dictionary to itself in the store's setDataTable mutation once any new keys and values have been added or removed from the current existing dictionary.

@jarmoza jarmoza marked this pull request as draft January 26, 2023 21:27
@jarmoza jarmoza added landing page data store maint:refactor Simplifying or restructuring existing code or documentation. feat:improve Incremental, user facing improvements of an existing feature. labels Jan 26, 2023
@jarmoza jarmoza requested a review from surchs January 26, 2023 22:10
@jarmoza jarmoza marked this pull request as ready for review January 26, 2023 22:11
@jarmoza
Copy link
Contributor Author

jarmoza commented Jan 26, 2023

@surchs Two further notes.

  1. Cypress component tests are all passing except the index page one. Will address this in the upcoming PR for adjusting tests (resolving refactor page test setup #309, adjust index page tests #310, adjust categorization page tests #311)
  2. Can you provide any feedback as to how this branch was created/merged? My workflow was to branch off of https://github.com/neurobagel/annotation_tool/tree/jarmoza-315 to continue development while awaiting feedback for its PR, then to merge changes to the dev branch into this one before proceeding with its own PR submission.

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.

Thanks for the PR @jarmoza. Changes look good, take a look at the tool-nav change, it doesn't look right to me.

pages/index.vue Show resolved Hide resolved
store/index.js Show resolved Hide resolved
components/tool-navbar.vue Outdated Show resolved Hide resolved
@surchs
Copy link
Contributor

surchs commented Jan 26, 2023

@surchs Two further notes.

1. Cypress component tests are all passing except the index page one. Will address this in the upcoming PR for adjusting tests (resolving [refactor page test setup #309](https://github.com/neurobagel/annotation_tool/issues/309), [adjust index page tests #310](https://github.com/neurobagel/annotation_tool/issues/310), [adjust categorization page tests #311](https://github.com/neurobagel/annotation_tool/issues/311))

Sounds good. If you know why the index page one is currently failing, I'd include the fix here just so we keep the component tests green. If it's more involved, then let's do the fix with the e2e ones (that's how I undersand your comment).

2. Can you provide any feedback as to how this branch was created/merged? My workflow was to branch off of https://github.com/neurobagel/annotation_tool/tree/jarmoza-315 to continue development while awaiting feedback for its PR, then to merge changes to the dev branch into this one before proceeding with its own PR submission.

I am not super clear what has happened here. My guess would be that you did something like this:

  1. Merge the previous PR (jarmoza-315) into dev
  2. Merge your local copy of dev into jarmoza-314 (here) before pulling the new merge commit now on origin/dev
  3. Then adding the missing changes from the merged PR into the already created local branch manually
  4. possibly missing something in step 3.

Does any of this sound plausible? We could also chat about it more on Monday with the rest of the team, I think we haven't really finished discussing our GH workflow last time.

@jarmoza
Copy link
Contributor Author

jarmoza commented Jan 26, 2023

@surchs

  1. I'll take a look at the e2e test again
  2. That sounds plausible, although re: 2, I was almost positive I had pulled down the changes to the remote dev branch.

In any case, I wanted to see how this workflow would work. Good practice.

@surchs
Copy link
Contributor

surchs commented Jan 27, 2023

@jarmoza: I took another look because I wasn't really sure either. You are right, you had pulled the recent PR in the dev branch. I think what may have happened is that when you merged dev into jarmoza-314 there was a merge conflict because you might have already made changes to the same line (in tool-navbar.vue) on both jarmoza-314 and on the dev_... branch. So my best guess is that you accidentally kept the conflicting change from jarmoza-314 and dismissed the one from the dev branch, thus reverting this line to what it was before the merge.

It's tricky - good thing for us all to discuss again I think - I would also like to get better intuitions when reading a tricky log.

@jarmoza
Copy link
Contributor Author

jarmoza commented Jan 27, 2023

Okay, @surchs. We're good to go with the index page component test now. The data dictionary being used and tests needed to be updated – since it now contains userProvided and annotated by default. (The previous data in the beforeEach for table and dictionary were moved as it was used for one test and not the rest.)

While I was determining what was wrong with the tests, I went ahead and cleaned up the file - generalized beforeEach input data, removed extraneous setup data, and did some formatting.

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.

Looks great @jarmoza. I left a comment for a left-over thing in the tool-navbar that should be removed. Agreed, this is ready to merge! 🎉

components/tool-navbar.vue Outdated Show resolved Hide resolved
pages/index.vue Show resolved Hide resolved
@jarmoza jarmoza merged commit 3995a38 into dev_components_talk_to_store_directly Jan 30, 2023
@surchs surchs deleted the jarmoza-314 branch January 30, 2023 15:40
@jarmoza jarmoza restored the jarmoza-314 branch January 30, 2023 16:19
@jarmoza jarmoza deleted the jarmoza-314 branch January 30, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data store feat:improve Incremental, user facing improvements of an existing feature. landing page maint:refactor Simplifying or restructuring existing code or documentation.
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants