Shared data tab refactor by marySalvi · Pull Request #894 · Kitware/dive

marySalvi · 2021-08-16T14:53:57Z

fixes #837

Adds 'Browse Data and 'Shared with Me' tabs
Adds Data Table to display the "Shared with Me' Data

…efactor

* Fixes for location * Location fixes * Prehydrate to prevent duplicate requests * Fix issue with event subscriber

subdavis

Few small changes.

Also, in order to make this change, the docs need to be updated. Mostly Sharing data with teams on the docs/Web-Version.md page needs to mention this new tab. The way you create shares is the same, but there should be a mention of where to find the items shared with you.

subdavis · 2021-09-13T13:24:49Z

client/platform/web-girder/api/dataset.service.ts

+      offset,
+      sort,
+      sortDir,
+      published: false,


might want to add this as an optional param?

Updated all params to be optional to match server-side.

subdavis · 2021-09-13T13:28:18Z

client/platform/web-girder/router.ts

+            {
+              path: 'shared',
+              name: 'shared',
+              component: DataShared,


need to add beforeEnter to this route as well, or possible just on the parent or grandparent?

Would it make sense to do a global beforeEach here?

I don't think it makes a difference. You can either put a beforeEnter in 2 places or you can check inside of a global beforeEach that the next route isn't the login route to prevent an infinite redirect.

I kinda prefer beforeEnter because if you do the beforeEach thing, you're sorta migrating some of the scope of the router config into function logic, but that's just me.

OK that makes sense. I don't personally have a preference so I will add the beforeEnter. It does look as though it needs to be added to each individual route, not the parent.

subdavis · 2021-09-13T13:32:11Z

client/platform/web-girder/store/Location.ts

-      const newPath = getPathFromLocation(location);
-      if (newPath !== router.currentRoute.path) {
-        router.push(newPath);
+    async hydrate({ commit }, location: LocationType) {


Add a comment that explains that hydrate populates the full girder model on location change if it's missing.

subdavis · 2021-09-13T13:41:07Z

client/platform/web-girder/views/DataBrowser.vue

+    }
+
+    function isAnnotationFolder(item: GirderModel) {
+      // TODO: update to check for other info


Is this todo still valid? What else would we check for?

subdavis · 2021-09-13T13:50:35Z

client/platform/web-girder/views/DataShared.vue

+    watch(tableOptions, updateOptions, {
+      deep: true,
+    });
+
+    updateOptions();


edit: removed suggestion.

You'll have to merge main again and run yarn to install latest dependeicies.

I retract this comment, watchEffect is still behaving unpredictably and is probably unsafe here. Please leave as-is.

subdavis · 2021-09-13T13:54:54Z

client/platform/web-girder/views/JobsTab.vue

    }

-    onBeforeUnmount(() => {
+    onBeforeUnmount(async () => {


this function does not need to be async.

subdavis · 2021-09-13T13:58:04Z

client/platform/web-girder/views/ShareTab.vue

+    const { getters } = store;
+
+    const clearSelected = () => {
+      locationStore.selected = [];


Do not mutate store from outside module definition. Use commit to invoke mutation. Vuex's typescript definitions are still really lacking.

store.commit('Location/setSelected', [])

I've had success in a few projects now using https://github.com/paroi-tech/direct-vuex. In my iterated experience, the strong typing overrides concerns about the wrapperness or the non-standardness of the library. Something to think about. I can hook you up with the engineers who actually were managing its usage, just let me know.

I like the looks of that (though I still generally dislike vuex). Any refactoring to use a new library like this should probably be a separate PR, but we should look into direct-vuex.

subdavis · 2021-09-13T14:21:57Z

client/platform/web-girder/views/ShareTab.vue

+      Browse Data
+    </v-tab>
+    <v-tab :to="{name: 'shared'}">
+      <v-icon class="mr-2">


There's a problem here where the width of the icon is dynamic, which causes a race condition with the calcuation of the underline active tab style. Soemtimes it ends up in the wrong place because the icon hasn't rendered yet.

You can solve this by forcing a fixed with on the icon. replace class="mr-2" with class="tab-icon" and define the new style as scoped.

<style scoped> .tab-icon { width: 28px; margin-right: 10px; } </style>

Great catch.

subdavis

Looks awesome!

One other small comment about adding a more helpful message for the case where there's no data
A simple documentation update

After those two small things, I think we should be good to merge.

subdavis · 2021-09-21T17:54:21Z

client/platform/web-girder/views/DataShared.vue

+        Launch Annotator
+      </v-btn>
+    </template>
+  </v-data-table>


<template #no-data> <span class="pr-4">No datasets have been shared with you yet.</span> <a href="https://kitware.github.io/dive/Web-Version/#sharing-data-with-teams">Learn more about sharing</a> </template>

I support aggressively linking to documentation. Can we add this slot here?

Definitely. I think this is a great idea.

subdavis

🎉

Mary Salvi and others added 13 commits July 27, 2021 09:03

adds shared tab and data table

ab7ee3c

WIP refactor

096d6db

Works!

fe9c8b2

Add shared with me endpoint

779583e

progress on sharedTab refactor

01e9b7c

fixes size formatter issue

a1d8c46

Merge branch 'main' of github.com:Kitware/dive into shared-data-tab-r…

741c92c

…efactor

Remove empty stub

a19f8dd

Add 'user' to query

265c9b1

Switch key to owner_login

04ae340

Change to ownerLogin

325884a

Girder Total Count header and sortDir

5441003

finishing up shared data tab

2b31711

marySalvi requested review from BryonLewis and subdavis August 16, 2021 14:53

subdavis and others added 5 commits August 19, 2021 14:23

next()

affe526

fixing merge conflicts

2b0aab8

Fix server CI failures

d2b89e8

Fixes for location (#913)

f04876a

* Fixes for location * Location fixes * Prehydrate to prevent duplicate requests * Fix issue with event subscriber

minor cleanup

b463f90

subdavis marked this pull request as ready for review September 10, 2021 23:27

subdavis suggested changes Sep 13, 2021

View reviewed changes

Mary Salvi added 2 commits September 17, 2021 16:22

merge from main

2b51e9a

minor edits and cleanup

2ae538c

subdavis self-requested a review September 21, 2021 17:52

subdavis suggested changes Sep 21, 2021

View reviewed changes

updates docs and adds no data message

9156f5f

subdavis self-requested a review September 21, 2021 18:52

subdavis approved these changes Sep 21, 2021

View reviewed changes

marySalvi merged commit f6fe9c0 into main Sep 22, 2021

subdavis deleted the shared-data-tab-refactor branch September 22, 2021 14:05

subdavis mentioned this pull request Sep 22, 2021

[BUG] Error on unauthenticated route #945

Closed

Conversation

marySalvi commented Aug 16, 2021 • edited by subdavis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subdavis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subdavis Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subdavis Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marySalvi commented Aug 16, 2021 •

edited by subdavis

Loading

subdavis left a comment •

edited

Loading

subdavis Sep 20, 2021 •

edited

Loading

subdavis Sep 13, 2021 •

edited

Loading