Kitware / vaui

Apache License 2.0
8 stars 1 forks source link

Activity and Track with new format #14

Closed matthewma7 closed 6 years ago

matthewma7 commented 6 years ago

This PR adds support for activity; Allow toggling activity and track on left panel; Use new annotation file format; Allow playing video without annotation file; Bug fixes;

mwoehlke-kitware commented 6 years ago

Thank you for better commit messages! :smile:

matthewma7 commented 6 years ago

@mwoehlke-kitware Sure. Thanks for letting me know the preferred format.

aashish24 commented 6 years ago

you probably want to update the readme describing the necessary files now

matthewma7 commented 6 years ago

@mwoehlke-kitware Did you finish reviewing this PR?

matthewma7 commented 6 years ago

@mwoehlke-kitware And, since you probably already tried out the style changes. Is ok to ask you to push the changes to this branch?

mwoehlke-kitware commented 6 years ago

I haven't looked at the parsers yet...

aashish24 commented 6 years ago

@mwoehlke-kitware thanks for your review. I think we can fix few things you pointed out. Some more style related issues, lets do it a separate PR (I will file few items on the github that we can address once we have more time). If you have to find any bug or performance issues then we will address then right away.

mwoehlke-kitware commented 6 years ago

@matthewma7, do you prefer to address the minor items in this PR or in a follow-up? (In either case, please do not add substantial new functionality to this PR; it will be harder to review that way.)

matthewma7 commented 6 years ago

@mwoehlke-kitware @aashish24 I pushed two commits to update some item mention in the review or yesterday's meeting. Let's merge this PR and address non-user-noticeable improvement in a separate PR.

mwoehlke-kitware commented 6 years ago

@matthewma7, I (force) pushed a slightly different version; PTAL. If it looks okay to you, I guess go ahead and merge this.

matthewma7 commented 6 years ago

@mwoehlke-kitware I think we should use bootstrap classes instead of specifying custom background color and border colors. And, placing at top right corner is overlapping with the girder message. Also, your change will cause the no annotation message be shown when while the annotation being loaded. I think we shouldn't force push once there is a PR. But to elaborate my point, I force pushed it back the previous state.

mwoehlke-kitware commented 6 years ago

FYI:

diff --git a/web_external/Viewer/index.jsx b/web_external/Viewer/index.jsx
index f0725cf..645aede 100644
--- a/web_external/Viewer/index.jsx
+++ b/web_external/Viewer/index.jsx
@@ -62,7 +62,10 @@ class Viewer extends Component {
                                 });
                             }}
                             key={this.props.itemModel.id} />,
-                        this.state.ready && this.props.itemModel && !this.props.annotationGeometryContainer && <div className='no-annotation-message alert alert-danger' key='no-annotation-message'><span>No annotation</span></div>,
+                        this.state.ready && !this.props.annotationGeometryContainer &&
+                          <div className='no-annotation-message'>
+                            <span>no annotations</span>
+                          </div>,
                         <div className='control' key='control'>
                             <div className='buttons btn-group'>
                                 {/* <button className='fast-backword btn btn-default' disabled={true}>

Checking this.props.itemModel here is redundant.

diff --git a/web_external/Viewer/style.styl b/web_external/Viewer/style.styl
index b5e0d26..992f254 100644
--- a/web_external/Viewer/style.styl
+++ b/web_external/Viewer/style.styl
@@ -17,11 +17,14 @@

   .no-annotation-message
     position absolute
-    top 4px
-    left 4px
+    top 3px
+    right 3px
     z-index 1
-    padding 5px
-  
+    padding 0 0.5em
+    background rgba(255, 224, 224, 0.5)
+    border 1px solid rgba(160, 80, 80, 0.5)
+    color #400
+
   .control
     padding-top 3px

Please don't use pixels for text spacing. We have enough problems with layouts not resizing and don't need to add more. Also, 3px is the containing element's padding; at 4px, the watermark ends up inside the view by 1px.

aashish24 commented 6 years ago

@matthewma7 @mwoehlke-kitware if there are no major issues, I would like this branch to get merged. We can address other minor issues (style or anything less critical) later in a separate PR.

aashish24 commented 6 years ago

@mwoehlke-kitware and @matthewma7 agree that we will merge this branch.

aashish24 commented 6 years ago

I have created a new issue #17 to address @mwoehlke-kitware