Add all icons and add new Icon component #221

lyzadanger · 2021-10-21T13:26:41Z

This PR tackles two important tasks:

Getting all known standard icons into this package
Updating with a new Icon component to alleviate a few longstanding issues

1. Consolidating and exporting Icons

The first commit here brings in the source of all of the icons in the old pattern library collection as well as a few scattered icons from our applications.

It provides a new module, icons, so that external apps can use these icons. An external application could do something like:

import { Icon, registerIcon } from '@hypothesis/frontend-shared';
import { annotate, annotateAlt, arrowDown } from '@hypothesis/frontend-shared/icons';

const annotateIcon = registerIcon('annotate', annotate);
const annotateAltIcon = registerIcon('annotateAlt', annotateAlt);
const arrowDownIcon = registerIcon('arrowDown', arrowDown);

// ...

<Icon name={annotateIcon} />

2. `Icon` component

The new Icon component has a few aims, while not deviating too much from the existing SvgIcon implementation:

Fix the casing of the component (SvgIcon doesn't match our conventions).
Conform to the props API for shared components (classes, containerClasses, e.g.)
Alleviate some styling issues

Regarding the styling issues that I believe this relieves, let's see if I can articulate.

SvgIcon (Background)

The SvgIcon component sets classes on the wrapping span. This results in one of two possibilities:

<span className="Hyp-SvgIcon"> — in this (default) case, display is set to flex. This has a side effect of being somewhat unexpected, and also resets flex layouts in unexpected ways. To get around that, we implemented:
<span className="Hyp-SvgIcon--inline"> — which is the result if one sets the prop inline={true}. In this case, display is set to inline. This prevents the component from resetting flex and puts it the inline flow, but inline elements cannot have width and height set reliably. The common result from this is a few phantom pixels:

In the example above, the desired width is 16x16px (1em here) but the height ends up being 18px. This is an artifact of the inline display.

With SvgIcon, the author can provide a className prop, which will apply class(es) to the SVG element, but there is no way to manipulate the styling on the wrapping span, which has led to pain over time.

SvgIcon doesn't give any sizing cues to the icon, leaving that to the author to wrangle by overriding default styling using the className prop.

Icon (new component)

The Icon component takes a different default styling approach. It leaves browser defaults in place for the wrapping span element (inline), and sets the svg element to have block display. It defaults to a 1em sizing, which makes the icon size scale to the current textual context by default.

The result is that the component renders inline and does not incur any block-level distractions (margins, padding, unexpectedly) or interruptions of flex layouts.

Authors can control styles on both the SVG element itself (via classes) and/or the wrapping span (containerClasses). The inline prop is no longer needed.

See it in play

A basic representation of all of the icons, using the new Icon component, can be seen on this branch in a new page in the pattern library. It's basic, but a starting point:

Fixes #151
Fixes #218

Remove mis-named `collapsed.svg`

Add a new Icon component to supersede SvgIcon in the future. It solves several challenges with SvgIcon, including: 1. Naming convention/casing 2. Layout challenges to do with `display` and inability to customize wrapping element classes in the past 3. Consistency with props API (`classes`, `containerClasses`)

robertknight · 2021-10-21T14:33:02Z

This has a side effect of being somewhat unexpected, and also resets flex layouts in unexpected ways. To get around that, we implemented:

I decided to investigate who was the guilty party here 😛. hypothesis/client@d2a8e58

lyzadanger · 2021-10-21T14:35:13Z

I decided to investigate who was the guilty party here 😛. hypothesis/client@d2a8e58

I've never been so glad to be the idiot :) . Saves me an hour or two of proving that it shouldn't be that way.

esanzgar

As I understand, after introducing the this new Icon component, we will phase out SvgIcon over time.

Should we mark the SvgIcon component as deprecated?

images/icons/refresh.svg

src/components/test/Icon-test.js

src/components/Icon.js

esanzgar · 2021-10-25T13:22:44Z

src/components/test/Icon-test.js

+import { availableIcons, registerIcons, registerIcon } from '../SvgIcon';
+import { Icon } from '../Icon';


Suggested change

import { availableIcons, registerIcons, registerIcon } from '../SvgIcon';

import { Icon } from '../Icon';

import { Icon } from '../Icon';

import { availableIcons, registerIcons, registerIcon } from '../SvgIcon';

It feels to me that the functions declare in SvgIcon belong to the Icon.

Yes, I agree, assuming we accept that Icon is the "icon component of record" going forward. There's some follow-up work here, which I'll summarize in a separate comment.

esanzgar · 2021-10-25T13:24:01Z

src/components/test/Icon-test.js

+  });
+
+  it("sets the element's content to the content of the SVG", () => {
+    const container = document.createElement('div');


container could be set on the beforeEach for all tests.

esanzgar · 2021-10-25T13:36:58Z

src/pattern-library/components/patterns/IconFoundations.js

+import { Card, Icon, registerIcon } from '../../../';
+import * as iconSrc from '../../../icons';
+
+const icons = {};


In general, I prefer the use of Maps.

Can you expand a little?

esanzgar · 2021-10-25T14:11:19Z

src/pattern-library/components/patterns/IconFoundations.js

+    <Library.Page title="Icons">
+      <Library.Pattern title="Hypothesis icon set">
+        <div className="LibraryGrid">
+          {Object.keys(icons).map(iconName => (


Suggested change

{Object.keys(icons).map(iconName => (

{Object.entries(icons).map(([iconName, iconSymbol]) => (

or if you use a Map:

{[...icons].map(([iconName, iconSymbol])

images/icons/annotate-alt.svg

src/pattern-library/routes.js

esanzgar · 2021-10-25T15:31:13Z

styles/components/Icon.scss

+  width: 1em;
+  height: 1em;


If a developer wants to make the SVG larger than 16x16 pixels, how should he/she do it? Modifying the container style?

One of two ways:

Do something that sets the inherited font size to the desired icon size, by e.g. applying a style to a containing element.

Use the classes prop on Icon to apply a class style with width-height overrides.

Presumably we will also have some utility styles specifically for sizing icons/text in the near future, too.

Remove unused classnames from SVG icons, leaving only those classes that are directly used by applications.

codecov · 2021-10-26T14:31:53Z

Codecov Report

Merging #221 (6a214d4) into main (3b0cbdc) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #221   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        16    +1     
  Lines          243       260   +17     
  Branches        89        95    +6     
=========================================
+ Hits           243       260   +17

Impacted Files	Coverage Δ
src/components/Icon.js	`100.00% <100.00%> (ø)`

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 3b0cbdc...6a214d4. Read the comment docs.

lyzadanger · 2021-10-26T14:39:10Z

Plan for follow-up:

Version package after this lands and dogfood the new Icon system with LMS and/or the client
If Icon seems like the way forward, deprecate SvgIcon and move icon-registration functions into Icon
Add pattern-library page specifically documenting usage of Icon component

I'll track these items separately.

lyzadanger · 2021-10-26T14:42:27Z

@esanzgar All of your feedback comments have shown up twice in this thread, so I hope I responded to at least the important ones and haven't missed anything major! Thanks!

lyzadanger added 2 commits October 21, 2021 07:55

Add complete Hypothesis icon set

ac6577d

Remove mis-named `collapsed.svg`

lyzadanger requested a review from esanzgar October 21, 2021 14:40

esanzgar approved these changes Oct 25, 2021

View reviewed changes

Remove classnames and XML declaration from SVG icons

6a214d4

Remove unused classnames from SVG icons, leaving only those classes that are directly used by applications.

lyzadanger merged commit 6849edf into main Oct 26, 2021

lyzadanger deleted the icon-export branch October 26, 2021 14:42

lyzadanger mentioned this pull request Oct 26, 2021

Rename SvgIcon component to SVGIcon #119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all icons and add new Icon component #221

Add all icons and add new Icon component #221

lyzadanger commented Oct 21, 2021 •

edited

Loading

robertknight commented Oct 21, 2021

lyzadanger commented Oct 21, 2021

esanzgar left a comment

esanzgar Oct 25, 2021

esanzgar Oct 25, 2021

lyzadanger Oct 26, 2021

esanzgar Oct 25, 2021

esanzgar Oct 25, 2021

lyzadanger Oct 26, 2021

esanzgar Oct 25, 2021

esanzgar Oct 25, 2021

lyzadanger Oct 26, 2021

lyzadanger Oct 26, 2021

codecov bot commented Oct 26, 2021

lyzadanger commented Oct 26, 2021

lyzadanger commented Oct 26, 2021

		import { availableIcons, registerIcons, registerIcon } from '../SvgIcon';
		import { Icon } from '../Icon';

	{Object.keys(icons).map(iconName => (
	{Object.entries(icons).map(([iconName, iconSymbol]) => (

Add all icons and add new Icon component #221

Add all icons and add new Icon component #221

Conversation

lyzadanger commented Oct 21, 2021 • edited Loading

1. Consolidating and exporting Icons

2. Icon component

SvgIcon (Background)

Icon (new component)

See it in play

robertknight commented Oct 21, 2021

lyzadanger commented Oct 21, 2021

esanzgar 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

codecov bot commented Oct 26, 2021

Codecov Report

lyzadanger commented Oct 26, 2021

lyzadanger commented Oct 26, 2021

lyzadanger commented Oct 21, 2021 •

edited

Loading

2. `Icon` component