Convert stage 3 into a single file#44
Conversation
siddhantwahal
requested review from
chamshop,
jonathanrocher and
prabhuramachandran
jonathanrocher
left a comment
jonathanrocher
left a comment
There was a problem hiding this comment.
This looks good, but I am making suggestions as to what this stage represent and how we could make a couple of versions around it to build a progression. Feel free to share yoour thoughts, and merge if you want. My comments can be done in subsequent PRs.
| figure = Instance(Figure) | ||
|
|
||
| view = View( | ||
| Item("model.filepath", style="readonly", show_label=False), |
There was a problem hiding this comment.
Do we want to make this not readonly so people can leverage the build_mpl_figure listener below?
There was a problem hiding this comment.
Or keep it readonly in 3.1 but make it "live" and add the @observe("model.filepath") in 3.2?
There was a problem hiding this comment.
Do we want to make this not readonly so people can leverage the build_mpl_figure listener below?
Great idea! This needed #44 to function properly so we can go ahead and do this now.
Or keep it readonly in 3.1 but make it "live" and add the @observe("model.filepath") in 3.2?
Interesting. Let me think a little more on how we could version this segment of the tutorial.
There was a problem hiding this comment.
Side note: We should track the difference between minor versions (i.e, 3.0, 3.1, 3.2), so that the presenter's job is a little easier. I'll try to add some "developer docs" with this PR.
|
@jonathanrocher I've split this into two stages, 3.0 and 3.1. Let me know if it's headed in the right direction! |
prabhuramachandran
left a comment
prabhuramachandran
left a comment
There was a problem hiding this comment.
I like this, my only request is that the plotting code also be rolled into a method on the model initially, but if you and @jonathanrocher feel strongly that it should not be there, this is fine as at least the code is in an if __name__... block.
Thanks for the review @prabhuramachandran! I'm merging this for now, but happy to come back and add a plotting method if that's what we decide in #42. |
|
Actually, @jonathanrocher, did you want to take another look? |
jonathanrocher
left a comment
jonathanrocher
left a comment
There was a problem hiding this comment.
LGTM. 1 suggestion to reduce the number of things to introduce. I totally get that this is a personal choice, but I think we need to remain opinionated so we figure out the minimum number of concepts to get to a full and scalable app in 4h. So I would like to cutout mercilessly everything we can since we know we will overload people...
|
|
||
| view = View( | ||
| UItem("model.filepath"), | ||
| UItem("figure", editor=MplFigureEditor()), |
There was a problem hiding this comment.
I wouldn't introduce UItem and stick to Item, but use its show_label=False, to reduce the number of things we have to explain.
There was a problem hiding this comment.
Totally agreed!
3 participants
This PR converts stage 3 into a single file.
In stage 2 (#42), we're adding a traits-equipped version of the script with an
ImageFilemodel. In this PR, we extend that simple script with a traitsui view forImageFile,ImageFileView.Model and views for
ImageFolderhave been removed. Thus, theImageFolderwill be introduced for the first time in Stage 5.1.