Add config option to display a clean onboarding look to users. #580

sheetaluk · 2017-11-15T17:35:10Z

No description provided.

robertknight · 2017-11-15T17:48:17Z

src/sidebar/templates/sidebar-tutorial.html

+    <li class="sidebar-tutorial__list-item sidebar-tutorial__list-item--is-theme-clean">
+      <p class="sidebar-tutorial__list-item-content">
+        View annotations through your profile
+        <i class="h-icon-account ng-scope"></i>


What is the ng-scope for here?

codecov · 2017-11-15T17:48:19Z

Codecov Report

Merging #580 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #580      +/-   ##
==========================================
+ Coverage   90.93%   90.99%   +0.06%     
==========================================
  Files         132      132              
  Lines        5337     5499     +162     
  Branches      928      967      +39     
==========================================
+ Hits         4853     5004     +151     
- Misses        484      495      +11

Impacted Files	Coverage Δ
src/sidebar/components/svg-icon.js	`100% <ø> (ø)`	⬆️
src/annotator/config/index.js	`100% <ø> (ø)`	⬆️
src/sidebar/host-config.js	`100% <ø> (ø)`	⬆️
src/sidebar/components/sidebar-tutorial.js	`90% <100%> (+1.11%)`	⬆️
src/sidebar/tabs.js	`100% <0%> (ø)`	⬆️
src/sidebar/components/annotation.js	`89.11% <0%> (+0.17%)`	⬆️
src/sidebar/root-thread.js	`98.88% <0%> (+0.4%)`	⬆️
src/sidebar/components/sidebar-content.js	`89.91% <0%> (+0.43%)`	⬆️
src/sidebar/components/annotation-header.js	`88.88% <0%> (+0.65%)`	⬆️
src/sidebar/components/selection-tabs.js	`86.95% <0%> (+3.62%)`	⬆️

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 09e5414...2070d93. Read the comment docs.

robertknight · 2017-11-15T17:48:46Z

src/styles/sidebar-tutorial.scss

+
+.sidebar-tutorial__list-item--is-theme-clean::before {
+  content: counter(sidebar-tutorial__list) ".";
+  display: table-cell; /* aha! */


What is this for? (ie. What is the "aha" ;) ?)

robertknight · 2017-11-15T17:49:52Z

src/styles/sidebar-tutorial.scss

+  border: none;
+  border-radius: 3px;
+  color: #fff;
+  display: -ms-flexbox;


CSS property values should be unprefixed (ie. display: flex). -ms-flexbox is a legacy value for IE 10/11. Our CSS preprocessor will add prefixes for the browsers we support. The exceptions are a few rare properties that only exist in one browser.

robertknight · 2017-11-15T17:52:46Z

src/styles/sidebar-tutorial.scss

 .sidebar-tutorial__list-item-content {
  margin-top: 1em; /* Put vertical space in-between list items, equal to the
                      space between normal paragraphs.
                      Note: This also puts the same amount of space above the
                      first list item (i.e. between the list and whatever's
                      above it). */
 }
+
+.sidebar-tutorial__list-item-annotate {
+  background-color: #b9d9fa;


Is this just for testing purposes, do we have variables that we'll eventually use here?

sheetaluk · 2017-11-15T19:37:31Z

@robertknight I cleaned up all the placeholders and copy-paste errors. Please take a look!

See hypothesis/product-backlog#351

robertknight · 2017-11-16T10:08:09Z

Note for future reference: For testing purposes in a local dev environment, the only way the sidebar tutorial can be re-enabled for a user who has dismissed it currently is via a direct DB update:

docker run -it --link postgres:postgres --rm postgres sh -c 'exec psql -h "$POSTGRES_PORT_5432_TCP_ADDR" -p "$POSTGRES_PORT_5432_TCP_PORT" -U postgres '

postgres=# update "user" set sidebar_tutorial_dismissed = false;

robertknight · 2017-11-16T10:16:20Z

This is what the combination of all the new UI customization options currently looks like on the publisher account test site:

If this option is enabled on its own for Hypothesis it looks like this:

robertknight · 2017-11-16T10:17:44Z

src/sidebar/templates/sidebar-tutorial.html

+      <div class="sidebar-tutorial__list-item-content">
+        Select some text to
+          <span class="sidebar-tutorial__list-item-annotate">annotate</span>
+          <svg-icon width="3px" height="10px" class="sidebar-tutorial__list-item-cursor" name="'cursor'"></svg-icon>


The width and height properties here are redundant and can be removed since we're now applying the styling in CSS.

robertknight · 2017-11-16T10:21:00Z

src/sidebar/templates/sidebar-tutorial.html

+    <li class="sidebar-tutorial__list-item sidebar-tutorial__list-item--is-theme-clean">
+      <div class="sidebar-tutorial__list-item-content sidebar-tutorial__list-item-content--is-theme-clean">
+        Create page level notes
+        <button class="sidebar-tutorial__list-item-new-note-btn" h-branding="ctaBackgroundColor">


Hmm ... this isn't actually clickable so it might be a bit surprising that it has the affordances of a button (eg. a cursor pointer).

- Use a variable instead of a hardcoded color for the "New note" button in the sidebar tutorial. - Remove unused `width` and `height` attributes. The icon's size is set via `width` and `height` properties in CSS.

robertknight

There were a couple of very minor code issues. In the interests of getting this out in a client release ASAP for testing I've pushed a commit with fixes for them.

I do think we should revisit the naming of all the new config options before we commit to supporting them, but we can do that while eLife are playing around with it.

robertknight · 2017-11-16T10:22:27Z

src/styles/sidebar-tutorial.scss

+}
+
+.sidebar-tutorial__list-item-new-note-btn {
+  background-color: #626262;


This should use the $color-dove-gray variable for consistency with other elements that are styled using the same palette element from the color palette.

robertknight · 2017-11-16T10:26:24Z

src/annotator/config/index.js

@@ -23,6 +23,7 @@ function configFrom(window_) {
    disableToolbarHighlightsBtn: settings.hostPageSetting('disableToolbarHighlightsBtn'),
    disableToolbarNewNoteBtn: settings.hostPageSetting('disableToolbarNewNoteBtn'),
    disableBucketBar: settings.hostPageSetting('disableBucketBar'),
+    enableCleanOnboardingTheme: settings.hostPageSetting('enableCleanOnboardingTheme'),


Perhaps for both consistency with how we are handling the "New note" button and also to make it clear that this isn't yet an officially supported setting, we ought to name it something like enableExperimentalNewOnboarding? 🤔 .

The new-look onboarding actually works fine whether or not the "clean" theme is being used, so perhaps we can consider it a separate thing.

The new note button in the onboarding tutorial shouldn't be a button because it isn't clickable. It is there to teach the user what to look for to create a new note. Turn the button element into a span with the same styling as the new note button which appears in the 'Page Notes' tab. See hypothesis/product-backlog#351 and #580 (comment)

sheetaluk added the WIP label Nov 15, 2017

sheetaluk force-pushed the onboarding branch 2 times, most recently from 4e4fb78 to b19c719 Compare November 15, 2017 17:42

robertknight reviewed Nov 15, 2017

View reviewed changes

sheetaluk force-pushed the onboarding branch 2 times, most recently from 3d6f6c1 to 7ca8450 Compare November 15, 2017 19:35

Add config option to display a clean onboarding look to users.

ccde896

See hypothesis/product-backlog#351

sheetaluk force-pushed the onboarding branch from 7ca8450 to ccde896 Compare November 15, 2017 23:04

sheetaluk removed the WIP label Nov 15, 2017

robertknight reviewed Nov 16, 2017

View reviewed changes

Minor CR fixups

2070d93

- Use a variable instead of a hardcoded color for the "New note" button in the sidebar tutorial. - Remove unused `width` and `height` attributes. The icon's size is set via `width` and `height` properties in CSS.

robertknight approved these changes Nov 16, 2017

View reviewed changes

robertknight merged commit 15cf8c4 into master Nov 16, 2017

robertknight deleted the onboarding branch November 16, 2017 10:45

sheetaluk mentioned this pull request Nov 16, 2017

Change button tag to span for the minimal onboarding look. #582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config option to display a clean onboarding look to users. #580

Add config option to display a clean onboarding look to users. #580

sheetaluk commented Nov 15, 2017

robertknight Nov 15, 2017

codecov bot commented Nov 15, 2017 •

edited

Loading

robertknight Nov 15, 2017 •

edited

Loading

robertknight Nov 15, 2017

robertknight Nov 15, 2017

sheetaluk commented Nov 15, 2017

robertknight commented Nov 16, 2017

robertknight commented Nov 16, 2017

robertknight Nov 16, 2017

robertknight Nov 16, 2017

robertknight left a comment

robertknight Nov 16, 2017

robertknight Nov 16, 2017

Add config option to display a clean onboarding look to users. #580

Add config option to display a clean onboarding look to users. #580

Conversation

sheetaluk commented Nov 15, 2017

robertknight Nov 15, 2017

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2017 • edited Loading

Codecov Report

robertknight Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

robertknight Nov 15, 2017

Choose a reason for hiding this comment

robertknight Nov 15, 2017

Choose a reason for hiding this comment

sheetaluk commented Nov 15, 2017

robertknight commented Nov 16, 2017

robertknight commented Nov 16, 2017

robertknight Nov 16, 2017

Choose a reason for hiding this comment

robertknight Nov 16, 2017

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

robertknight Nov 16, 2017

Choose a reason for hiding this comment

robertknight Nov 16, 2017

Choose a reason for hiding this comment

codecov bot commented Nov 15, 2017 •

edited

Loading

robertknight Nov 15, 2017 •

edited

Loading