Feature/246 add proptypes and default vals #273
Conversation
…lopment' of https://github.com/MetaCell/geppetto-meta into feature/246_add_proptypes_and_default_vals
… into feature/246_add_proptypes_and_default_vals
There was a problem hiding this comment.
Thank you very much @Muhaddatha.
I think if we solve the ListViewer problem and add a couple of descriptions here and there we are good to go. If something is not clear in the changes I requested let me know.
geppetto.js/geppetto-ui/src/connectivity-viewer/ConnectivityComponent.js
Show resolved
Hide resolved
|
One more thing that you didn't touch but it might make sense to do in the context of this issue:
I would also like to bump issue #1 to our attention because flex-layout component will be the only one with dummy prop-types |
Ok, I looked at the plots props but its fields are a bit different. Maybe I am looking at the wrong file. Is this the file you are talking about? |
That's the correct file. Indeed the inner props seem to be wrongly displayed. If adding a description for x, y and lineOptions doesn't solve it we can create a new issue. Let me know |
| onShiftClick: 'goToPoint', | ||
| onDoubleClick: 'goToPoint', | ||
| showDownloadButton: false, | ||
| }; | ||
|
|
||
|
|
||
| DicomViewer.propTypes = { |
There was a problem hiding this comment.
I believe onClick, onCtrlClick, onShiftClick, onDoubleClick shouldn't be any but rather a string (one of 'goToPoint', 'goToSingleView', 'togglMode'), or a function.
There was a problem hiding this comment.
I also changed 'togglMode' to 'toggleMode'. I made the example DicomViewer match this too.
This is how the DicomViewer Props look like with the new format:
Let me know if this is the right way.
There was a problem hiding this comment.
Regarding the change from togglMode to toggleMode I think that's all right. I pushed a new commit to fix one thing you forgot to change. The idea of those 'modes' is to allow the user to choose some default onX strategies. That works because we have those strategies implemented as methods inside the DicomViewer component. When the user chooses 'toggleMode' we map it to the method toggleMode. Because this is using eval (which is something that we probably should change as it, typically, is not very safe/pretty) the 'mode' needs to match with the name of the method.
New format looks very nice in paper. I don't like how it looks in the showcase but I think we can address that in #285.
There was a problem hiding this comment.
Looks good to me.
I added the things I don't like to card #285.
Final word on you @enicolasgomez
No description provided.