Add svg-icon to frontend-shared package

- Add babel presets for frontend-shared to transpile js/jsx
- Add `build` script to transpile frontend-shared and output sourcemaps and transpiled js code to /lib
- Add frontend-shared /src/index.js as a way to serve as a root import location for the frontend-shared lib
- Fix up frontend-shared/package.json so publishing will work as expecting and include only intended files
- Add a browserify config in the frontend-shared package.json file so that driving client will be able to bundle it and still import svg and html files that are nested inside /frontend-shared/src
- Add svg-icon to serve as the first shared js component
- Move several .svg files into the lib that are relaxant for testing svg-icon that also seem share-worthy
- Make modifications to ensure that typechecking, linting, and testing all work as expected and ignore /lib folder
- Change karma config so that tests will automatically run on /frontend-shared, driven from the client's gulp command
- Added new gulp tasks to help with building and linking the frontend-shared package so that is it fully transparent with existing tooling
- Add gulp-babel and gulp-sourcemaps packages to help with build tasks for /frontend-shared that are necessary for symlinking / building the package
- Used new SvgIcon sidebar/index.js as a tester. This requires making two copies of registerIcons while we make the transition. Replacing all call sites of SvgIcon would be too large for an already overwhelming commit

.eslintignore

@@ -3,3 +3,4 @@ build/**
 **/coverage/**
 docs/_build/*
 dev-server/static/**/*.js
+frontend-shared/lib/**
\ No newline at end of file

.gitignore

@@ -4,7 +4,7 @@ coverage/
 docs/_build/
 
 # The client uses Yarn rather than npm to manage the lockfile.
-package-lock.json
+./package-lock.json
 
 # SSL certificate and key.
 .tlscert.pem

.prettierignore

@@ -3,4 +3,5 @@
 build/
 coverage/
 docs/
-dev-server/static/*
\ No newline at end of file
+dev-server/static/*
+lib/
\ No newline at end of file

frontend-shared/.eslintrc

+{
+  "parserOptions": {
+    "sourceType": "module"
+  }
+}

frontend-shared/.gitignore

+node_modules/
+lib/
+

frontend-shared/README.md

@@ -2,6 +2,11 @@
 
 A package of resources for Hypothesis front-end applications.
 
+#### Requirements
+
+- preact
+- browserify
+
 ### Usage
 
 ```
@@ -14,6 +19,12 @@ $ npm install @hypothesis/frontend-shared --save
 @use "@hypothesis/frontend-shared/styles/mixins" as mixins;
 ```
 
+#### In JS
+
+```js
+import { SvgIcon } from '@hypothesis/frontend-shared';
+```
+
 License
 -------
 

frontend-shared/babel.config.json

+{
+  "presets": [
+    [
+      "@babel/env", {
+        "targets": {
+          "chrome": "40",
+          "firefox": "38",
+          "safari": "9",
+          "ie": "11"
+        },
+        "useBuiltIns": "usage",
+        "corejs": "3.6.5"
+      }
+    ],
+    [
+      "@babel/preset-react", {
+        "pragma": "createElement"
+      }
+    ]
+  ]
+}
\ No newline at end of file

frontend-shared/images/icons/arrow-left.svg

+<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" aria-hidden="true" focusable="false" class="Icon Icon--arrow-left"><g fill-rule="evenodd"><rect fill="none" stroke="none" x="0" y="0" width="16" height="16"></rect><path fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M7 12L3 8l4-4M4 8h9-9z"></path></g></svg>

frontend-shared/images/icons/arrow-right.svg

+<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" aria-hidden="true" focusable="false" class="Icon Icon--arrow-right"><g fill-rule="evenodd"><rect fill="none" stroke="none" x="0" y="0" width="16" height="16"></rect><path fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M9 4l4 4-4 4m3-4H3h9z"></path></g></svg>

frontend-shared/images/icons/check.svg

+<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" class="Icon Icon--check"><g fill-rule="evenodd"><rect fill="none" stroke="none" x="0" y="0" width="16" height="16"></rect><path fill="none" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M13 3L6 13 3 8"></path></g></svg>

frontend-shared/package.json

@@ -2,5 +2,34 @@
   "name": "@hypothesis/frontend-shared",
   "version": "1.0.0",
   "repository": "hypothesis/client/frontend-shared",
-  "dependencies": {}
+  "devDependencies": {
+    "@babel/cli": "^7.1.6",
+    "@babel/core": "^7.1.6",
+    "@babel/preset-env": "^7.1.6",
+    "@babel/preset-react": "^7.0.0"
+  },
+  "scripts": {
+    "build": "npx babel src --out-dir lib --source-maps --ignore '**/test'"
+  },
+  "files": [
+    "lib",
+    "styles",
+    "images"
+  ],
+  "main": "./lib/index.js",
+  "browserify": {
+    "transform": [
+      [
+        "stringify",
+        {
+          "appliesTo": {
+            "includeExtensions": [
+              ".html",
+              ".svg"
+            ]
+          }
+        }
+      ]
+    ]
+  }
 }

frontend-shared/src/index.js

+module.exports = {
+  ...require('./svg-icon'),
+};

frontend-shared/src/svg-icon.js

+import classnames from 'classnames';
+import { createElement } from 'preact';
+import { useLayoutEffect, useRef } from 'preact/hooks';
+import propTypes from 'prop-types';
+
+/**
+ * Object mapping icon names to SVG markup.
+ *
+ * @typedef {Object.<string,string>} IconMap
+ */
+
+/**
+ * @template T
+ * @typedef {import("preact/hooks").Ref<T>} Ref
+ */
+
+/**
+ * Map of icon name to SVG data.
+ *
+ * @type {IconMap}
+ */
+let iconRegistry = {};
+
+/**
+ * @typedef SvgIconProps
+ * @prop {string} name - The name of the icon to display.
+ *   The name must match a name that has already been registered using the
+ *   `registerIcons` function.
+ * @prop {string} [className] - A CSS class to apply to the `<svg>` element.
+ * @prop {boolean} [inline] - Apply a style allowing for inline display of icon wrapper.
+ * @prop {string} [title] - Optional title attribute to apply to the SVG's containing `span`.
+ */
+
+/**
+ * Component that renders icons using inline `<svg>` elements.
+ * This enables their appearance to be customized via CSS.
+ *
+ * This matches the way we do icons on the website, see
+ * https://github.com/hypothesis/h/pull/3675
+ *
+ * @param {SvgIconProps} props
+ */
+export function SvgIcon({ name, className = '', inline = false, title = '' }) {
+  if (!iconRegistry[name]) {
+    throw new Error(`Icon name "${name}" is not registered`);
+  }
+  const markup = iconRegistry[name];
+
+  const element = /** @type {Ref<HTMLElement>} */ (useRef());
+  useLayoutEffect(() => {
+    const svg = element.current.querySelector('svg');
+
+    // The icon should always contain an `<svg>` element, but check here as we
+    // don't validate the markup when it is registered.
+    if (svg) {
+      svg.setAttribute('class', className);
+    }
+  }, [
+    className,
+    // `markup` is a dependency of this effect because the SVG is replaced if
+    // it changes.
+    markup,
+  ]);
+
+  const spanProps = {};
+  if (title) {
+    spanProps.title = title;
+  }
+
+  return (
+    <span
+      className={classnames('svg-icon', { 'svg-icon--inline': inline })}
+      dangerouslySetInnerHTML={{ __html: markup }}
+      ref={element}
+      {...spanProps}
+    />
+  );
+}
+
+SvgIcon.propTypes = {
+  name: propTypes.string.isRequired,
+  className: propTypes.string,
+  inline: propTypes.bool,
+  title: propTypes.string,
+};
+
+/**
+ * Register icons for use with the `SvgIcon` component.
+ *
+ * @param {IconMap} icons
+ * @param {Object} options
+ *  @param {boolean} [options.reset] - If `true`, remove existing registered icons.
+ */
+export function registerIcons(icons, { reset = false } = {}) {
+  if (reset) {
+    iconRegistry = {};
+  }
+  Object.assign(iconRegistry, icons);
+}
+
+/**
+ * Return the currently available icons.
+ *
+ * To register icons, don't mutate this directly but call `registerIcons`
+ * instead.
+ *
+ * @return {IconMap}
+ */
+export function availableIcons() {
+  return iconRegistry;
+}

frontend-shared/src/test/svg-icon-test.js

+import { createElement, render } from 'preact';
+
+import { SvgIcon, availableIcons, registerIcons } from '../svg-icon';
+
+describe('SvgIcon', () => {
+  // Tests here use DOM APIs rather than Enzyme because SvgIcon uses
+  // `dangerouslySetInnerHTML` for its content, and that is not visible in the
+  // Enzyme tree.
+
+  // Global icon set that is registered with `SvgIcon` outside of these tests.
+  let savedIconSet;
+
+  beforeEach(() => {
+    savedIconSet = availableIcons();
+
+    registerIcons(
+      {
+        'arrow-left': require('../../images/icons/arrow-left.svg'),
+        'arrow-right': require('../../images/icons/arrow-right.svg'),
+      },
+      { reset: true }
+    );
+  });
+
+  afterEach(() => {
+    registerIcons(savedIconSet, { reset: true });
+  });
+
+  it("sets the element's content to the content of the SVG", () => {
+    const container = document.createElement('div');
+    render(<SvgIcon name="arrow-left" />, container);
+    assert.ok(container.querySelector('svg'));
+  });
+
+  it('throws an error if the icon name is not registered', () => {
+    assert.throws(() => {
+      const container = document.createElement('div');
+      render(<SvgIcon name="unknown" />, container);
+    }, 'Icon name "unknown" is not registered');
+  });
+
+  it('does not set the class of the SVG by default', () => {
+    const container = document.createElement('div');
+    render(<SvgIcon name="arrow-left" />, container);
+    const svg = container.querySelector('svg');
+    assert.equal(svg.getAttribute('class'), '');
+  });
+
+  it('sets the class of the SVG if provided', () => {
+    const container = document.createElement('div');
+    render(<SvgIcon name="arrow-left" className="thing__icon" />, container);
+    const svg = container.querySelector('svg');
+    assert.equal(svg.getAttribute('class'), 'thing__icon');
+  });
+
+  it('retains the CSS class if the icon changes', () => {
+    const container = document.createElement('div');
+    render(<SvgIcon name="arrow-left" className="thing__icon" />, container);
+    render(<SvgIcon name="arrow-right" className="thing__icon" />, container);
+    const svg = container.querySelector('svg');
+    assert.equal(svg.getAttribute('class'), 'thing__icon');
+  });
+
+  it('sets a default class on the wrapper element', () => {
+    const container = document.createElement('div');
+    render(<SvgIcon name="arrow-left" />, container);
+    const wrapper = container.querySelector('span');
+    assert.isTrue(wrapper.classList.contains('svg-icon'));
+    assert.isFalse(wrapper.classList.contains('svg-icon--inline'));
+  });
+
+  it('appends an inline class to wrapper if `inline` prop is `true`', () => {
+    const container = document.createElement('div');
+    render(<SvgIcon name="arrow-left" inline={true} />, container);
+    const wrapper = container.querySelector('span');
+    assert.isTrue(wrapper.classList.contains('svg-icon'));
+    assert.isTrue(wrapper.classList.contains('svg-icon--inline'));
+  });
+
+  it('sets a title to the containing `span` element if `title` is present', () => {
+    const container = document.createElement('div');
+    render(<SvgIcon name="arrow-left" title="Open menu" />, container);
+    const wrapper = container.querySelector('span');
+    assert.equal(wrapper.getAttribute('title'), 'Open menu');
+  });
+
+  it('sets does not set a title on the containing `span` element if `title` not present', () => {
+    const container = document.createElement('div');
+    render(<SvgIcon name="arrow-left" />, container);
+    const wrapper = container.querySelector('span');
+    assert.notOk(wrapper.getAttribute('title'));
+  });
+});

gulpfile.js

@@ -15,6 +15,10 @@ const through = require('through2');
 
 const createBundle = require('./scripts/gulp/create-bundle');
 const createStyleBundle = require('./scripts/gulp/create-style-bundle');
+const {
+  buildFrontendSharedJs,
+  linkFrontendShared,
+} = require('./scripts/gulp/frontend-shared');
 const manifest = require('./scripts/gulp/manifest');
 const serveDev = require('./dev-server/serve-dev');
 const servePackage = require('./dev-server/serve-package');
@@ -142,6 +146,19 @@ gulp.task(
   })
 );
 
+gulp.task('build-frontend-shared-js', buildFrontendSharedJs);
+
+gulp.task('link-frontend-shared', linkFrontendShared);
+
+gulp.task(
+  'build-frontend-shared',
+  gulp.series('build-frontend-shared-js', 'link-frontend-shared')
+);
+
+gulp.task('watch-frontend-shared', () => {
+  gulp.watch('./frontend-shared/src/**', gulp.series(buildFrontendSharedJs));
+});
+
 const cssBundles = [
   // H
   './src/styles/annotator/annotator.scss',
@@ -314,7 +331,9 @@ gulp.task(
     'watch-css',
     'watch-fonts',
     'watch-images',
-    'watch-manifest'
+    'watch-manifest',
+    'watch-frontend-shared',
+    'link-frontend-shared'
   )
 );
 

package.json

@@ -10,7 +10,6 @@
     "@babel/core": "^7.1.6",
     "@babel/preset-env": "^7.1.6",
     "@babel/preset-react": "^7.0.0",
-    "@hypothesis/frontend-shared": "./frontend-shared",
     "@octokit/rest": "^18.0.0",
     "@sentry/browser": "^5.6.2",
     "approx-string-match": "^1.1.0",
@@ -49,9 +48,11 @@
     "fetch-mock": "9",
     "focus-visible": "^5.0.0",
     "gulp": "^4.0.0",
+    "gulp-babel": "^8.0.0",
     "gulp-changed": "^4.0.0",
     "gulp-rename": "^2.0.0",
     "gulp-replace": "^1.0.0",
+    "gulp-sourcemaps": "^3.0.0",
     "hammerjs": "^2.0.4",
     "karma": "^6.0.1",
     "karma-browserify": "^8.0.0",

scripts/gulp/frontend-shared.js

+'use strict';
+
+const cp = require('child_process');
+const fs = require('fs');
+const gulp = require('gulp');
+const babel = require('gulp-babel');
+const sourcemaps = require('gulp-sourcemaps');
+
+const buildFrontendSharedJs = () => {
+  // There does not appear to be a simple way of forcing gulp-babel to use a config
+  // file. Load it up as JSON and pass it in manually.
+  const babelConfig = JSON.parse(
+    fs.readFileSync('./frontend-shared/babel.config.json')
+  );
+
+  return (
+    gulp
+      .src('frontend-shared/src/*')
+      // Transpile the js source files and write the output in the frontend-shared/lib dir.
+      // Additionally, add the sourcemaps into the same dir.
+      .pipe(sourcemaps.init())
+      .pipe(babel(babelConfig))
+      .pipe(sourcemaps.write('.'))
+      .pipe(gulp.dest('frontend-shared/lib'))
+  );
+};
+
+const linkFrontendShared = done => {
+  // Setup a symlink from the client to the frontend-shared package.
+  cp.spawn('yarn', ['link'], {
+    env: process.env,
+    cwd: './frontend-shared',
+  }).on('exit', () => {
+    cp.spawn('yarn', ['link', '@hypothesis/frontend-shared'], {
+      env: process.env,
+      stdio: 'inherit',
+    }).on('exit', () => {
+      done();
+    });
+  });
+};
+
+module.exports = {
+  buildFrontendSharedJs,
+  linkFrontendShared,
+};

src/karma.config.js

@@ -30,7 +30,11 @@ if (process.env.RUNNING_IN_DOCKER) {
 }
 
 module.exports = function (config) {
-  let testFiles = ['**/test/*-test.js', '**/integration/*-test.js'];
+  let testFiles = [
+    '**/test/*-test.js',
+    '**/integration/*-test.js',
+    '../frontend-shared/**/test/*-test.js',
+  ];
 
   if (config.grep) {
     const allFiles = testFiles
@@ -85,6 +89,7 @@ module.exports = function (config) {
       './boot/polyfills/*.js': ['browserify'],
       './sidebar/test/bootstrap.js': ['browserify'],
       '**/*-test.js': ['browserify'],
+      '../frontend-shared/**/*-test.js': ['browserify'],
       '**/*-it.js': ['browserify'],
     },
 
@@ -94,6 +99,15 @@ module.exports = function (config) {
         [
           'babelify',
           {
+            presets: [
+              '@babel/preset-env',
+              [
+                '@babel/preset-react',
+                {
+                  pragma: 'createElement',
+                },
+              ],
+            ],
             extensions: ['.js'],
             plugins: [
               'mockable-imports',

src/sidebar/components/help-panel.js

+import { SvgIcon } from '@hypothesis/frontend-shared';
 import { createElement } from 'preact';
 import { useCallback, useMemo, useState } from 'preact/hooks';
 import propTypes from 'prop-types';
@@ -8,7 +9,6 @@ import { withServices } from '../service-context';
 import VersionData from '../helpers/version-data';
 
 import SidebarPanel from './sidebar-panel';
-import SvgIcon from '../../shared/components/svg-icon';
 import Tutorial from './tutorial';
 import VersionInfo from './version-info';
 

src/sidebar/index.js

@@ -93,8 +93,11 @@ function setupFrameSync(frameSync, store) {
 }
 
 // Register icons used by the sidebar app (and maybe other assets in future).
-import { registerIcons } from '../shared/components/svg-icon';
+import { registerIcons as registerIconsDeprecated } from '../shared/components/svg-icon';
+import { registerIcons } from '@hypothesis/frontend-shared';
 import iconSet from './icons';
+
+registerIconsDeprecated(iconSet); // Temporary until we replace this module
 registerIcons(iconSet);
 
 // The entry point component for the app.

src/styles/mixins/buttons.scss

@@ -18,6 +18,7 @@
  * And pass 'my-component-button' as the `className` prop to `Button`.
  */
 @use "@hypothesis/frontend-shared/styles/mixins/focus";
+
 @use "./layout";
 @use "./utils";
 @use "../variables" as var;

src/styles/mixins/forms.scss

 @use "@hypothesis/frontend-shared/styles/mixins/focus";
+
 @use "./utils";
 @use "../variables" as var;
 

src/styles/sidebar/components/menu-item.scss

-@use "sass:color";
 @use "@hypothesis/frontend-shared/styles/mixins/focus";
+
+@use "sass:color";
 @use "../../mixins/layout";
 @use "../../mixins/utils";
 @use "../../variables" as var;

src/styles/sidebar/components/menu.scss

-@use "../../mixins/buttons";
 @use "@hypothesis/frontend-shared/styles/mixins/focus";
+
+@use "../../mixins/buttons";
 @use "../../mixins/layout";
 @use "../../mixins/molecules";
 @use "../../mixins/utils";

src/styles/sidebar/components/selection-tabs.scss

 @use "@hypothesis/frontend-shared/styles/mixins/focus";
+
 @use "../../mixins/buttons";
 @use "../../mixins/layout";
 @use "../../mixins/utils";

src/tsconfig.json

@@ -12,12 +12,14 @@
     "target": "ES2020"
   },
   "include": [
-    "**/*.js"
+    "**/*.js",
+    "../frontend-shared/src/**/*.js",
   ],
   "exclude": [
     // Tests are not checked.
     "**/test/**/*.js",
     "test-util/**/*.js",
     "karma.config.js",
+    "../frontend-shared/src/test/**/*.js",
   ]
 }