Don't show view switcher until annotations received #481
Conversation
|
I haven't added any tests for this. I want to get it out to production (its behind a feature flag) first and see whether it achieves the aim of reducing the popping effect / how it looks on production |
2d54fb1 to
7241aca
Compare
Codecov Report
@@ Coverage Diff @@
## master #481 +/- ##
==========================================
+ Coverage 89.9% 90.09% +0.18%
==========================================
Files 134 134
Lines 5232 5400 +168
Branches 903 941 +38
==========================================
+ Hits 4704 4865 +161
- Misses 528 535 +7
Continue to review full report at Codecov.
|
| @@ -34,6 +42,7 @@ module.exports = { | |||
| isLoading: '<', | |||
| isWaitingToAnchorAnnotations: '<', | |||
| selectedTab: '<', | |||
| showViewSwitcher: '<', | |||
There was a problem hiding this comment.
The list of properties in the bindings section are inputs to the component, ie. attributes in the list provided to <view-switcher> in templates that use the component. showViewSwitcher is an internal controller method however, so it doesn't need to be here.
| getState: sinon.stub().returns({ | ||
| frames: [ | ||
| { | ||
| isAnnotationFetchComplete: true, |
There was a problem hiding this comment.
A short comment here would be useful to explain what this is for.
Don't show the view switcher until the first batch of annotations has been received. (This first batch will be all the annotations, unless there are more than 200 annotations of the page.) This is an attempt to reduce "popping" caused by the annotation switcher suddenly changing a brief time after first render, when annotations are loaded: hypothesis/product-backlog#327 (comment)
7241aca to
f240476
Compare
| @@ -19,6 +19,14 @@ module.exports = { | |||
| return features.flagEnabled('orphans_tab'); | |||
| }; | |||
|
|
|||
| this.showViewSwitcher = function() { | |||
| var frame = annotationUI.getState().frames[0]; | |||
There was a problem hiding this comment.
I'll note that this list may be empty early in the life of the application when the top-level frame has not yet connected. In that case this will be an out-of-bounds access, which returns undefined. This works fine in JS, but I generally prefer to explicitly handle the case.
Remove the (feature-flagged) view switcher component. We missed the chance to strike while the iron was hot on this one - neither Dawa or I have time anymore to work on further design iterations. I don't want to have dead code living behind a feature flag so I'm removing it. The code will still be in git if we ever want to revive it. Perhaps these tabs can be re-designed as part of a larger re-design of the sidebar in the future. I'll leave the GitHub issue about the usability issues with the existing selection tabs open: hypothesis/product-backlog#327 For the record the main remaining issues with this were: 1. An undesirable visual popping happens when the view switcher loads on pages that have a lot of annotations. This also happens with the existing selection tabs but is arguably more visible with the view switcher. 2. When there are only two tabs (no orphans) the visual design of the tabs doesn't make it as immediately obvious which is the currently selected tab. The other tab may be mistaken for the selected one. The TODO list was here: hypothesis/product-backlog#327 (comment) The pull requests for this view switcher were: #429 #481 #482 hypothesis/product-backlog#327
Don't show the view switcher until the first batch of annotations has
been received. (This first batch will be all the annotations, unless
there are more than 200 annotations of the page.)
This is an attempt to reduce "popping" caused by the annotation switcher
suddenly changing a brief time after first render, when annotations are
loaded:
hypothesis/product-backlog#327 (comment)