Replace selection tabs with view switcher (feature flagged) #465

seanh · 2017-06-26T13:58:47Z

Replace the annotations, page notes and orphans selection tabs at the top of the sidebar:

With a new view switcher control:

See also:

Issue about selection tabs usability problems: The client's "Annotations / Page Notes / Orphans" tabs have usability issues product-backlog#327
Original discussion PR for the view switcher idea: Replace Annotations / Page Notes / Orphans selection tabs with a standard view switcher component #429
h pr to add view switcher feature flag: Add view-switcher feature flag h#4581

As with the previous selection tabs, the counts of annotations and page notes are only shown when there are > 0 annotations or page notes. Here it is with 0 page notes:

And with 0 annotations or page notes:

As with the previous selection tabs, the orphans tab only appears when there are orphans. Here it is when there are orphans but no annotations or page notes:

With both annotations and orphans but no page notes:

Because the widths of the tabs change according to the length of the content inside the tab (the label, e.g. "Annotations" plus the count, e.g. 12) the button width changes when the count goes from 0 to 1, or 1 to 0, or 9 to 10, etc. But this doesn't look as bad as I thought it would and Dawa commented that the button width should be dynamic in this way in order for the spacing inside the buttons to be consistent:

@dawariley :

Adjusted spacing between count and tab name
Made tab width dynamic

I suggest we review and merge this based on the code, then we can enable the feature flag for Dawa so she can see it in production, then we can remove the feature flag once Dawa has approved it.

seanh · 2017-06-26T14:22:22Z

P.S. I haven't tested this in different browsers or on mobile, we should do that before removing the feature flag, but I think we can still merge this particular PR without that testing. I've added a checklist of things to do before moving the feature flag in a comment on the issue: hypothesis/product-backlog#327 (comment)

robertknight

I've given this a test with a prod browser extension on a few pages.

A UX question I have is whether people might think that the brighter tab is the active one, rather than the grey tab. I think that was my initial reaction. This is something we can discuss after this is merged.

I've added a few comments and questions about the code.

robertknight · 2017-06-28T09:32:45Z

src/sidebar/components/test/view-switcher-test.js

+describe('selectionTabs', function () {
+  before(function () {
+    angular.module('app', [])
+      .component('selectionTabs', require('../selection-tabs'));


References to selectionTabs and selection-tabs.js need replacing with viewSwitcher / view-switcher.js.

Oops, I think I forgot to update the tests

robertknight · 2017-06-28T09:38:14Z

src/sidebar/components/sidebar-content.js

@@ -325,6 +325,8 @@ function SidebarContentController(
    annotationUI.clearSelectedAnnotations();
    annotationUI.selectTab(selectedTab);
  };
+
+  this.viewSwitcherEnabled = features.flagEnabled('view-switcher');


In theory feature flags can change after the <sidebar-content> is instantiated. The feature flags are stored in the app state, so a way to avoid this is to set viewSwitcherEnabled in the annotationUI.subscribe callback near the top of this function. You can use isFeatureEnabled(state, featureName) selector to extract the state of a feature flag from the app state.

robertknight · 2017-06-28T09:55:07Z

src/styles/view-switcher.scss

+
+  border: 1px solid $gray-lighter;
+
+  touch-action: manipulation;


What is touch-action for here?

Hmm. I think I copied it from Bootstrap's button group CSS! But on a google what it does is: 1. Disables double-tapping on the button to zoom in on the button; 2. Makes tapping the button faster as browser doesn't need to delay generation of click event to wait and see if you're doing a double-tap. (To really test this I need to get my dev client working on my phone which for some reason it isn't right now.)

Makes tapping the button faster as browser doesn't need to delay generation of click event to wait and see if you're doing a double-tap.

That's not relevant in most web apps nowadays because browsers disable the double tap gesture by default if the page is mobile-ready. In the case of our sidebar the story could be more complex.

I would suggest we remove this until we are sure it does something we want and we've actually tested it.

robertknight · 2017-06-28T09:57:10Z

src/styles/view-switcher.scss

+  cursor: pointer;
+  user-select: none;
+
+  -webkit-appearance: button;


What effects does -webkit-appearance have vs. not using it here and do we also need -moz-appearance for Firefox? If using just appearance on its own, does that get vendor prefixed by our existing autoprefixer infrastructure?

Hmm, yeah I don't think we want this. I'd be interested to test whether it does anything on mobile or desktop Safari but for now I'll remove it

robertknight · 2017-06-28T09:57:32Z

src/styles/view-switcher.scss

+}
+
+.view-switcher__tab:focus {
+  outline:0 !important;


What is the !important needed for here?

It doesn't seem to do anything, removed. Also added a -moz-focus-inner rule that stops Firefox from drawing a botted inner border inside the button when you click on it.

robertknight · 2017-06-28T09:57:55Z

src/styles/view-switcher.scss

+
+.view-switcher__tab:hover,
+.view-switcher__tab.is-selected {
+  background-color: #e6e6e6;


Do we have an existing color from our palette that we can use here?

To be honest, I don't think so. $grey-1 and $grey-2 are too light in my opinion (not different enough from the unselected colour) and then $grey-3 is way too dark. There's no example that I'm aware of of an old-style Hypothesis button or tab that has a depressed state like this one. I tried mine-shaft, dove-gray etc as well but didn't think any of them worked. (I can't remember where I picked this particular shade from originally, can't find it anywhere else in the app now!)

seanh · 2017-06-28T16:59:16Z

Just noticed that when the tabs are first rendered there are no numbers on them (because the client hasn't downloaded any annotations yet), then after a second the numbers appear (and the widths of the tabs change). On fast connections this is so fast you don't notice it, but on mobile connections it becomes noticeable here it is on a regular 2G connection:

I wonder if the solution is to not show the view switcher at all until the annotations are available?

This same problem exists with the current selection tabs as well, I just noticed it with the new view switcher, so I think it can be fixed as a separate issue.

seanh · 2017-06-28T17:52:45Z

I can see what you mean about maybe thinking the white tab is the selected one, when there are only two tabs.

seanh · 2017-06-28T17:59:21Z

I still want to test this across browsers and devices but apart from that all the comments are responded to.

codecov · 2017-06-28T19:13:21Z

Codecov Report

Merging #465 into master will decrease coverage by 0.04%.
The diff coverage is 77.77%.

@@            Coverage Diff            @@
##           master    #465      +/-   ##
=========================================
- Coverage   89.84%   89.8%   -0.05%     
=========================================
  Files         132     133       +1     
  Lines        5110    5128      +18     
  Branches      874     879       +5     
=========================================
+ Hits         4591    4605      +14     
- Misses        519     523       +4

Impacted Files	Coverage Δ
src/sidebar/components/sidebar-content.js	`89.67% <ø> (ø)`	⬆️
src/sidebar/components/view-switcher.js	`77.77% <77.77%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f3d331...6ab2c84. Read the comment docs.

dawachan · 2017-06-29T01:33:37Z

(Am failing at threaded inline responses to comments so appending mine to the end.)

Here's my thoughts:

Loading delay/resize issue:

I think it's best to not show the tabs until we have the annotation count as it doesn't look to re-load the tabs at a wider size, it appears glitchy. Also t seems redundant to have a switcher control, if there's nothing to switch between. So +1 to your solution @seanh and good job on catching this existing bug.

De(selected) tab colour

I think the white definitely visually pops more, and the grey tab visually recedes. Can you please try reducing the saturation of the black text on white tab to be a light grey as it will appear more 'disabled' than active, this might be enough to draw the users eye to the grey tab. Let's see how this looks.

If you could also show a version where the onRollover + active tab state are white, and the others grey, this would be helpful to see too, thank you!

robertknight · 2017-06-29T09:44:53Z

I wonder if the solution is to not show the view switcher at all until the annotations are available?

I think that will probably result in a more noticeable "pop" when the view switcher shows and shifts everything down, if the page has enough annotations that they are loaded in batches.

A slight variation on your suggestion would be to delay showing the switcher until at least the first batch of annotations have been received.

robertknight · 2017-06-29T09:47:29Z

src/styles/view-switcher.scss

+  // These are the exact margins required to vertically align the top of the
+  // view switcher with the top of the Hide Highlights button to its left,
+  // and the top of the first annotation card below the view switcher with the
+  // top of the New Page Note button to its left.


These comments are very helpful. If we can find a way to do the alignment "in code" via shared variables etc. that's even better.

Yep. I'm not sure exactly what would be required to achieve that though - for example I'm not sure whether the existing elements are aligned to a grid (and therefore the grid size could be a variable that we could use multiples of). If it's not aligned to a grid and some of the gaps are seemingly arbitrary then we might end up with a lot of difficult to name variables. So needs investigation.

robertknight

One small comment about touch-action. My general suggestion is that if you're not sure what effect something has, get rid of it!

I think this is ready to merge as is. @dawariley's feedback can be addressed in follow up PRs. The feature flag is currently turned on for all staff, we might want to enable it only for a smaller subset of us until that feedback is addressed.

If the 'view-switcher' feature flat is enabled then use the new view switcher component, otherwise uses the old selection tabs component. The view switcher component is just a placeholder for now.

seanh · 2017-06-29T10:41:29Z

Merged, I'll get this released and activated behind a feature flag soon. Lets continue the discussion on the issue hypothesis/product-backlog#327 (comment)

This was referenced Jun 26, 2017

Add view-switcher feature flag hypothesis/h#4581

Merged

Replace Annotations / Page Notes / Orphans selection tabs with a standard view switcher component #429

Closed

hypothesis deleted a comment from codecov bot Jun 26, 2017

robertknight requested changes Jun 28, 2017

View reviewed changes

robertknight self-assigned this Jun 28, 2017

seanh force-pushed the replace-selection-tabs-with-view-switcher branch from 3245328 to 2e5629f Compare June 28, 2017 17:49

robertknight reviewed Jun 29, 2017

View reviewed changes

robertknight approved these changes Jun 29, 2017

View reviewed changes

seanh mentioned this pull request Jun 29, 2017

The client's "Annotations / Page Notes / Orphans" tabs have usability issues hypothesis/product-backlog#327

Open

seanh added 2 commits June 29, 2017 11:23

Add new feature-flagged view-switcher

7290181

If the 'view-switcher' feature flat is enabled then use the new view switcher component, otherwise uses the old selection tabs component. The view switcher component is just a placeholder for now.

Implement the new view switcher HTML and CSS

6ab2c84

seanh force-pushed the replace-selection-tabs-with-view-switcher branch from 2e5629f to 6ab2c84 Compare June 29, 2017 10:25

seanh merged commit 595b182 into master Jun 29, 2017

seanh deleted the replace-selection-tabs-with-view-switcher branch June 29, 2017 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace selection tabs with view switcher (feature flagged) #465

Replace selection tabs with view switcher (feature flagged) #465

seanh commented Jun 26, 2017 •

edited

Loading

seanh commented Jun 26, 2017

robertknight left a comment

robertknight Jun 28, 2017

seanh Jun 28, 2017

seanh Jun 28, 2017

robertknight Jun 28, 2017

seanh Jun 28, 2017

robertknight Jun 28, 2017

seanh Jun 28, 2017

robertknight Jun 29, 2017

seanh Jun 29, 2017

robertknight Jun 28, 2017

seanh Jun 28, 2017

robertknight Jun 28, 2017

seanh Jun 28, 2017

robertknight Jun 28, 2017

seanh Jun 28, 2017 •

edited

Loading

seanh commented Jun 28, 2017

seanh commented Jun 28, 2017

seanh commented Jun 28, 2017

codecov bot commented Jun 28, 2017 •

edited

Loading

dawachan commented Jun 29, 2017

robertknight commented Jun 29, 2017

robertknight Jun 29, 2017

seanh Jun 29, 2017

robertknight left a comment

seanh commented Jun 29, 2017

Replace selection tabs with view switcher (feature flagged) #465

Replace selection tabs with view switcher (feature flagged) #465

Conversation

seanh commented Jun 26, 2017 • edited Loading

seanh commented Jun 26, 2017

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

seanh commented Jun 28, 2017

seanh commented Jun 28, 2017

seanh commented Jun 28, 2017

codecov bot commented Jun 28, 2017 • edited Loading

Codecov Report

dawachan commented Jun 29, 2017

robertknight commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

seanh commented Jun 29, 2017

seanh commented Jun 26, 2017 •

edited

Loading

seanh Jun 28, 2017 •

edited

Loading

codecov bot commented Jun 28, 2017 •

edited

Loading