girder / girder_web_components

Reusable Javascript and VueJS components for interacting with a Girder server.
https://gwc.girder.org
Apache License 2.0
16 stars 9 forks source link

Add support for browsing collection and user with DataBrowser #121

Closed matthewma7 closed 5 years ago

matthewma7 commented 5 years ago

This PR adds support of user and collection browsing to the DataBrowser and Breadcrumb components. This PR should meet all requirement of the issue and would resolve https://github.com/girder/girder_web_components/issues/119. This requires girder>=3.0.0a7.dev71

This PR introduces a concept of Root in terms of the location. concept Root includes 'root', 'users', 'collections'. ~The concept of location was using girder models of type user, collection, and folder. However, with the introduction Root, the location has been changed to has two properties, a type property, and an optional id property. The type can be 'root', 'users', 'collections', 'user', 'collection', or 'folder'. When the value is 'user', 'collection', or 'folder', it should has the id field.~ The location props will be set to {type: 'root'/'users'/'collections'} when the location is not a girder user, collection or folder.

This PR also makes location prop on DataBrowser optional, so the user of DataBrowser doesn't have to provide a location or make it sync.

The fact that Upsert-Folder and Upload dialog is not internalized in DataBrowser makes the App.vue of the demo a little bit awkward. And, also, I and Jeff think we should standardize boolean props, to make component easier to be used. I am thinking we could do that in following PRs.

2019-05-13_20-08-14

matthewma7 commented 5 years ago

@jeffbaumes @subdavis @brianhelba Any feedback? 😉

jeffbaumes commented 5 years ago

To capture some other discussion with @matthewma7 just now, the plan is to simplify the "home" navigation UI to just have a small home icon to the left of the globe icon that is visible if someone is logged in, that always goes to the user home.

matthewma7 commented 5 years ago

Added tests. This is ready for review.

subdavis commented 5 years ago

Thanks Matthew.

I still have a few small comments:

Request changes:

Discuss:

Thanks again for putting this PR together.

matthewma7 commented 5 years ago

Thanks, @subdavis for revisiting this. I think I already addressed #121 (comment)? I agree, create sounds more accurate.

About the conditional on the template, we can totally see other input on this thread. I personally am thinking since all of them are else if, they are essentially switch statement, which is not that branch complex. and should all have been covered, (but not tested? because no expect() written for them?), if you are concerned that they are not tested, do you think adding tests with wrapper.find would solve that?

matthewma7 commented 5 years ago

Cool! thank you for the reviewing and approval. @subdavis I'll merge this in one hour.