-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix Dataset Configurations #4845
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend looks good to me, I think this is a clean solution :) I just added a few minor comments.
Co-authored-by: Florian M <[email protected]>
…/webknossos into fix-dataset-configurations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! Also kudos for cleaning up the obsolete quality-setting and fighting through the front-end tests :)
I'm just a bit unclear about why it is necessary to send the dataset layers to the back-end when requesting the settings. Since the volume-vs-fallback-layer-specific logic is now duplicated, I'd prefer to refactor this a bit. See my other comment for context/details :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent :)
@fm3 Could you have a look at the backend again after the newest frontend changes. I don't know how to feel about the abstraction level in the |
…/webknossos into fix-dataset-configurations
ToDo:
|
…-dataset-configurations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend LGTM, great work 🎉 Thanks for going through all the renamings :) I think it is a lot more clear and consistent now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Great job making this beautiful 👍 See my two comments about validation + nesting, though :)
frontend/javascripts/dashboard/dataset/default_config_component.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/types/schemas/dataset_view_configuration_defaults.js
Outdated
Show resolved
Hide resolved
frontend/javascripts/types/schemas/dataset_view_configuration_defaults.js
Show resolved
Hide resolved
…-dataset-configurations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Looking forward to seeing this merged :)
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
[ ] Adapted wk-connect if datastore API changes[ ] Needs datastore update after deployment