AllenNeuralDynamics / aind-metadata-entry-js

Metadata entry using javascript
https://metadata-entry.allenneuraldynamics.org
MIT License
1 stars 1 forks source link

Feat 28 coordinates #36

Closed mekhlakapoor closed 1 year ago

mekhlakapoor commented 1 year ago

Customizes UI of coordinate fields in ephys session schema using UISchema and ObjectFieldTemplate. For now, the conditional for the uiSchema is in RenderForm.js itself, but can make a function uiSchemaSelection in SchemaHandlers.js once we have more than one uiSchema. Below pics are before/after for ccf coordinates, but it does the ui change for all coordinate pair entries (so lab coords and manipulator coords as well)

![Screen Shot 2022-10-26 at 10 16 21 AM](https://us

Screen Shot 2022-10-26 at 11 22 31 AM

er-images.githubusercontent.com/54870020/198105721-66f888d2-8ebc-4e58-ae54-8351131284f4.png)

dyf commented 1 year ago

@mekhlakapoor how hard would it be to hard-code the "direction" values to be AP - DV - ML so that they don't need to be selected?

jtyoung84 commented 1 year ago

@mekhlakapoor how hard would it be to hard-code the "direction" values to be AP - DV - ML so that they don't need to be selected?

@dyf Ideally that should be driven by the schema if the fields need to all be there to validate correctly. A solution can be hacked for specific fields to have defaults values in place, but it shouldn't be the long term solution.

dyf commented 1 year ago

@mekhlakapoor how hard would it be to hard-code the "direction" values to be AP - DV - ML so that they don't need to be selected?

@dyf Ideally that should be driven by the schema if the fields need to all be there to validate correctly.

Yeah, just not sure how to do that. I think the desired UI behavior is 3 number entry boxes with inferred AP/ML/DV directions. If that's not doable without creating a giant mess then this is fine IMO.

jtyoung84 commented 1 year ago

@mekhlakapoor how hard would it be to hard-code the "direction" values to be AP - DV - ML so that they don't need to be selected?

@dyf Ideally that should be driven by the schema if the fields need to all be there to validate correctly.

Yeah, just not sure how to do that. I think the desired UI behavior is 3 number entry boxes with inferred AP/ML/DV directions. If that's not doable without creating a giant mess then this is fine IMO.

My worry is that the ui is already a hack around the schema listing these coordinates as enums. We can add another hack to set the default values to AP/DV/ML. I feel like we can avoid this by updating the schema to follow the geographical coordinates example here: https://json-schema.org/learn/miscellaneous-examples.html

This example renders as:

Screenshot from 2022-10-26 11-52-45

mekhlakapoor commented 1 year ago

@mekhlakapoor how hard would it be to hard-code the "direction" values to be AP - DV - ML so that they don't need to be selected?

@dyf Ideally that should be driven by the schema if the fields need to all be there to validate correctly.

Yeah, just not sure how to do that. I think the desired UI behavior is 3 number entry boxes with inferred AP/ML/DV directions. If that's not doable without creating a giant mess then this is fine IMO.

My worry is that the ui is already a hack around the schema listing these coordinates as enums. We can add another hack to set the default values to AP/DV/ML. I feel like we can avoid this by updating the schema to follow the geographical coordinates example here: https://json-schema.org/learn/miscellaneous-examples.html

This example renders as:

Screenshot from 2022-10-26 11-52-45

@dyf @jtyoung84 the ui-hack might be possible by using uiSchema's enumDisabled feature and creating some hack to loop through and disable two directions per direction field and set a ui:placeholder for each) but it would definitely be pretty complicated for a UI task.

For now, I can use ui:description to add a message stating to only do one of each, and I'm willing to help change the schema so that we can better assess which method (either ui-hack or schema change) would be better to meet this request.

dyf commented 1 year ago

@mekhlakapoor how hard would it be to hard-code the "direction" values to be AP - DV - ML so that they don't need to be selected?

@dyf Ideally that should be driven by the schema if the fields need to all be there to validate correctly.

Yeah, just not sure how to do that. I think the desired UI behavior is 3 number entry boxes with inferred AP/ML/DV directions. If that's not doable without creating a giant mess then this is fine IMO.

My worry is that the ui is already a hack around the schema listing these coordinates as enums. We can add another hack to set the default values to AP/DV/ML. I feel like we can avoid this by updating the schema to follow the geographical coordinates example here: https://json-schema.org/learn/miscellaneous-examples.html This example renders as: Screenshot from 2022-10-26 11-52-45

@dyf @jtyoung84 the ui-hack might be possible by using uiSchema's enumDisabled feature and creating some hack to loop through and disable two directions per direction field and set a ui:placeholder for each) but it would definitely be pretty complicated for a UI task.

For now, I can use ui:description to add a message stating to only do one of each, and I'm willing to help change the schema so that we can better assess which method (either ui-hack or schema change) would be better to meet this request.

Works for me!

jtyoung84 commented 1 year ago

@mekhlakapoor how hard would it be to hard-code the "direction" values to be AP - DV - ML so that they don't need to be selected?

@dyf Ideally that should be driven by the schema if the fields need to all be there to validate correctly.

Yeah, just not sure how to do that. I think the desired UI behavior is 3 number entry boxes with inferred AP/ML/DV directions. If that's not doable without creating a giant mess then this is fine IMO.

My worry is that the ui is already a hack around the schema listing these coordinates as enums. We can add another hack to set the default values to AP/DV/ML. I feel like we can avoid this by updating the schema to follow the geographical coordinates example here: https://json-schema.org/learn/miscellaneous-examples.html This example renders as: Screenshot from 2022-10-26 11-52-45

@dyf @jtyoung84 the ui-hack might be possible by using uiSchema's enumDisabled feature and creating some hack to loop through and disable two directions per direction field and set a ui:placeholder for each) but it would definitely be pretty complicated for a UI task. For now, I can use ui:description to add a message stating to only do one of each, and I'm willing to help change the schema so that we can better assess which method (either ui-hack or schema change) would be better to meet this request.

Works for me!

If it's an easy fix, then go for it. I still don't think it's a sustainable idea to hack the ui to render a field to be something else instead of altering the underlying schema just to be that something else to begin with.

mekhlakapoor commented 1 year ago

A few new line issues need to be addressed

should be taken care of now