Skip to content
This repository has been archived by the owner on Feb 22, 2020. It is now read-only.

#31 Show directory tree of possible images #85

Merged
merged 1 commit into from Sep 9, 2017

Conversation

WGierke
Copy link
Contributor

@WGierke WGierke commented Sep 1, 2017

A file tree is rendered using Vue.js that should show the images the user can select from the local disk.

Description

I based this PR on rahit's work in #81 that adds a new endpoint to fetch images from. The changes render a file tree at the index page that visualizes the response from that new endpoint, assuming the endpoint would return the files in a certain structure.

Reference to official issue

This should address #31 .

Motivation and Context

It already adds some functionality in the front-end to render the structure of the available images. This is already done in Vue.js instead of using the Django template language.

How Has This Been Tested?

ImageAvailableApiView.get() is currently not implemented, yet. So currently, the tree only has a lonely root:
screenshot from 2017-09-02 00-41-58

However, assuming the view would return the following response

JsonResponse({'directories': [{'name': 'test_cases, no modules',
                      'children': ['test_case_001', 'test_case_002',
                                   'test_case_003']},
                     {'name': 'test_cases, benign',
                      'children': ['test_case_004', 'test_case_005',
                                   'test_case_006']},
                     {'name': 'test_cases, cancerous',
                      'children': ['test_case_007', 'test_case_008',
                                   'test_case_009']}]})

the picture referenced in issue #31 would be rendered:
screenshot from 2017-09-02 00-40-31

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well

@rahit
Copy link
Contributor

rahit commented Sep 2, 2017

Hi @WGierke . One thing that passed on my mind when I initially started working on the issue was - whether the directory structure will be fixed or it will be dynamic.

+-- root
     |-- Level 1
          |-- Level 2 directory 1
                |-- case 001
                |-- case 002
          |-- Level 2 directory 2
                |-- case 001
                |-- case 002

Do we want to grab all the files and directories under the sub directories as well? For instances, LIDC-IDRI data does not follow this structure. Nor the test images directory. If we move to another data source (or DICOM image server which is planned for production) the structure might be different.

But, if that is not the case, next step will be very easy. What do you think about the issue? I think @lamby can help us by guiding on this. :)

@WGierke WGierke force-pushed the 31_image_directory_tree branch 2 times, most recently from 7e2b283 to 708b3fe Compare September 2, 2017 07:21
},
fetchAvailableImages: function() {
var vm = this;
this.$http.get('/api/images/available').then(
Copy link
Contributor

Choose a reason for hiding this comment

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

{% url … %} here? :)

this.directoriesDict = response.body;
},
function() {
// error callback
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you file a new issue (referencing this one) regarding investigating a global error handler? Like, some kind of drop-down that will pop up on any error so we aren't having to implement it each time...

@WGierke WGierke mentioned this pull request Sep 3, 2017
2 tasks
@WGierke
Copy link
Contributor Author

WGierke commented Sep 3, 2017

Thanks @lamby , I added appropriate url tags and filed a new issue. What do you think of rahit's question - are we expecting that the API returns a directory structure deeper than two levels? That would most likely require us to render the file tree recursively. To resolve #31 so far I assumed that the tree should always look like the one in the picture of #31 (so only have two levels).

@WGierke WGierke force-pushed the 31_image_directory_tree branch 2 times, most recently from 7d8f139 to 0ed2f28 Compare September 3, 2017 18:31
@lamby
Copy link
Contributor

lamby commented Sep 4, 2017

are we expecting that the API returns a directory structure deeper than two levels

For now, lets go with no. MVP!

@WGierke
Copy link
Contributor Author

WGierke commented Sep 5, 2017

@lamby That sounds nice :) Are there any more remarks?

@lamby
Copy link
Contributor

lamby commented Sep 5, 2017

@WGierke No further comments here, am just seeing "This branch cannot be rebased due to conflicts". If you can fix that up I'll get it "live" :)

@WGierke
Copy link
Contributor Author

WGierke commented Sep 5, 2017

@lamby Thanks, I squashed the commits. Hopefully, merging should work now :)

@lamby
Copy link
Contributor

lamby commented Sep 6, 2017

@WGierke Oh dear I'm seeing conflicts with the base branch now.

Also by "squash", please don't squash unrelated commits - it's always a bad smell when you see the word "and" in a commit message; feel free to split those bits out.

@WGierke
Copy link
Contributor Author

WGierke commented Sep 6, 2017

@lamby Thanks, I'll stick to that in the future. The conflicts should now be resolved.

@lamby
Copy link
Contributor

lamby commented Sep 7, 2017

@WGierke Alas still seeing "This branch cannot be rebased due to conflicts" :) ^

@WGierke WGierke force-pushed the 31_image_directory_tree branch 2 times, most recently from daaee61 to d35a4a8 Compare September 7, 2017 22:58
@WGierke
Copy link
Contributor Author

WGierke commented Sep 7, 2017

@lamby conflicts are resolved again. I'm not fooling you - after merging other pull requests into the master this branch just gets conflicts again :)

@lamby lamby merged commit d6306ff into drivendataorg:master Sep 9, 2017
@lamby
Copy link
Contributor

lamby commented Sep 9, 2017

Merged.. @WGierke Did you make that ticket re a global eror handler...?

@WGierke
Copy link
Contributor Author

WGierke commented Sep 9, 2017

@lamby yes it should be described in #86

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants