Add icon widget #477

maier49 · 2018-02-16T16:00:27Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

Resolves #463

tomdye · 2018-02-16T16:05:29Z

src/icon/README.md

+import { w } from '@dojo/widget-core/d';
+
+w(Icon, {
+	type: 'downIcon'


can you add aria usage to the example please

tomdye · 2018-02-16T16:08:44Z

src/icon/icon.ts

+	}
+
+	constructor() {
+		super();


don't think you need this

tomdye · 2018-02-16T16:09:16Z

src/icon/icon.ts

+	protected render(): DNode {
+		const {
+			aria = {},
+			type = 'downIcon'


I don't think we should have a default for type

Should the property be required on the props then or just render without an icon type if it's not provided?

codecov · 2018-02-16T17:18:23Z

Codecov Report

Merging #477 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #477      +/-   ##
==========================================
+ Coverage   97.34%   97.36%   +0.01%     
==========================================
  Files          31       32       +1     
  Lines        2414     2431      +17     
  Branches      629      630       +1     
==========================================
+ Hits         2350     2367      +17     
  Misses          9        9              
  Partials       55       55

Impacted Files	Coverage Δ
src/text-input/index.ts	`99.03% <ø> (ø)`	⬆️
src/dialog/index.ts	`98.61% <100%> (-0.04%)`	⬇️
src/calendar/index.ts	`99.49% <100%> (ø)`	⬆️
src/button/index.ts	`100% <100%> (ø)`	⬆️
src/toolbar/index.ts	`91.04% <100%> (ø)`	⬆️
src/icon/index.ts	`100% <100%> (ø)`
src/select/index.ts	`96.21% <100%> (ø)`	⬆️
src/title-pane/index.ts	`98.3% <100%> (ø)`	⬆️
src/slide-pane/index.ts	`89.28% <100%> (-0.16%)`	⬇️
src/combobox/index.ts	`97.65% <100%> (ø)`	⬆️
... and 2 more

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 fcbb79c...29a1ad8. Read the comment docs.

smhigley

Nice first widget addition!

smhigley · 2018-02-16T18:50:00Z

src/icon/Icon.ts

+export class IconBase<P extends IconProperties = IconProperties> extends ThemedBase<P, null> {
+	private _onClick (event: MouseEvent) {
+		event && event.stopPropagation();
+		this.properties.onClick && this.properties.onClick();


We shouldn't be providing an onClick event, since icons aren't interactive -- they're not buttons, nothing indicates they can be clicked, and they are neither focusable nor keyboard-operable. If someone wants an icon to do something, they need to wrap it in a button or link.

Good point, missed that :)

smhigley · 2018-02-16T18:53:17Z

src/icon/Icon.ts

+		} = this.properties;
+
+		return v('i', {
+			...formatAriaProperties(aria),


It's good to include the aria props, but it might be worthwhile to also have an explicit property for hidden descriptive text, maybe like altText? If it's provided, we can leave off role="presentation" and include a visually hidden span with the text. If it's not provided, we can include role="presentation" and aria-hidden="true"

Now it's always setting aria-hidden="true" on the icon itself (unless explicitly overridden), never setting role="presentation" on anything, and includes a visually hidden with altText if provided.

smhigley · 2018-02-16T20:36:38Z

src/icon/Icon.ts

+ */
+export interface IconProperties extends ThemedProperties, CustomAriaProperties {
+	type: IconType;
+	altText?: string;


Yay! Could you just add it to typedoc and the customElement decorator?

smhigley · 2018-02-16T20:40:01Z

src/icon/Icon.ts

+		const {
+			aria = {
+				hidden: 'true'
+			},


I don't think we want to default to aria-hidden="true", because that will hide the alt text if provided. It should probably only be present if altText is missing.

As we talked about, the label is a sibling of the icon so we can safely always hide the icon.

smhigley · 2018-02-16T20:40:58Z

src/icon/README.md

+```
+
+By default the icon will be rendered with `role="presentation"` and
+`aria-hidden="true"`. Custom aria attributes can be specified to


Really quick word update to something like "The icon will be rendered with role="presentation" and
+aria-hidden="true" if altText is not provided"

Updated this, but with slightly different wording since we'll always have aria-hidden="true" regardless of whether altText is provided.

tomdye · 2018-02-16T20:39:12Z

src/button/Button.ts

@@ -134,7 +127,7 @@ export class ButtonBase<P extends ButtonProperties = ButtonProperties> extends T
 			'aria-pressed': typeof pressed === 'boolean' ? pressed.toString() : null
 		}, [
 			...this.getContent(),
-			popup ? this.renderPopupIcon() : null
+			popup ? w(Icon, { extraClasses: { root: css.addon }, type: 'downIcon' }) : null


this won't work as extraClasses is not themeable. You'll likely need to wrap it in a span or a div and pass the css.addon class to that instead.

Wrapped them in a span

tomdye · 2018-02-16T20:39:31Z

src/button/tests/unit/Button.ts

@@ -1,12 +1,13 @@
+


why the new line?

Formatting failure. Removed these from all the files they were in.

tomdye · 2018-02-16T20:39:59Z

src/button/tests/unit/Button.ts

-					role: 'presentation',
-					'aria-hidden': 'true'
-				})
+				w(Icon, { type: 'downIcon', extraClasses: { root: css.addon } })


same comment again re. wrapping the icon with something for the sake of css.addon

tomdye · 2018-02-16T20:42:40Z

src/icon/Icon.ts

+})
+export class IconBase<P extends IconProperties = IconProperties> extends ThemedBase<P, null> {
+	protected renderAltText(altText?: string) {
+		return altText && v('span', { classes: [ baseCss.visuallyHidden ] }, [ altText ]);


could the altText not just be a title attribute on the i tag? @smhigley ??

I think having a separate span is preferred.

tomdye · 2018-02-16T20:43:37Z

src/icon/Icon.ts

+	}
+}
+
+export default class Icon extends IconBase<IconProperties> {}


just export default class Icon

Not sure I follow. Do you want me to get rid of the base class?

@tomdye I'm not entirely sure if this is what you meant, but this pattern was adopted because of typing weirdness around lack of type narrowing when providing the generic for properties.

smhigley · 2018-02-16T22:54:37Z

src/icon/Icon.ts

+ *
+ * Properties that can be set on an Icon component
+ *
+ * @property type           Icon type, e.g. downIcon, searchIcon, etc.


I promise this is the very last thing: could you add altText after this?

nevermind! My view was out of date :)

smhigley

Just have the one comment for adding a description for altText for typedoc. Other than that, I'm good for you to merge this 👍

tomdye

Looks good, just one question about the IconType.

tomdye · 2018-02-19T10:55:33Z

src/calendar/Calendar.ts

@@ -375,13 +374,11 @@ export class CalendarBase<P extends CalendarProperties = CalendarProperties> ext
 	}

 	protected renderPagingButtonContent(type: Paging, labels: CalendarMessages): DNode[] {
-		const iconClass = type === Paging.next ? iconCss.rightIcon : iconCss.leftIcon;
+		const iconType = type === Paging.next ? 'rightIcon' : 'leftIcon';


could this be IconType.rightIcon ? Might be better using the exported type. There's a few instances like this within this PR.

Right now IconType is just a type, so there is no IconType.rightIcon.

tomdye · 2018-02-21T15:03:07Z

@maier49 this will require a change in @dojo/themes too as the icons filename has changed.

tomdye · 2018-02-21T15:26:57Z

@maier49 I raised dojo/themes#9 to go with this change.

tomdye · 2018-02-27T11:52:53Z

@maier49 looks like this is failing CI right now

tomdye · 2018-02-28T10:33:13Z

src/icon/README.md

+
+*Basic Example*
+```typescript
+import Icon from '@dojo/widgets/icon/index';


When people use our widgets in their own apps they don't need to use /index, this is just for our AMD test loaders sadly. I'm happy to let this go as it though and will update all readmes in a follow up PR

tomdye · 2018-02-28T10:37:32Z

src/icon/index.ts

+		const altTextNode = this.renderAltText(altText);
+		const icon = this.renderIcon(type, aria);
+
+		return v('span', { classes: this.theme(css.root) }, [


Do you think this really needs a renderIcon method? I can't see anyone ever extending it, it's simply an icon.
As for renderAltText, I think that it should only be called if there is altText.

return v('span', { classes: this.theme(css.root) }, [ v('i', { ...formatAriaProperties(aria), classes: this.theme([ css.icon, css[type] ]) }), altText ? this.renderAltText(altText) : null ]);

tomdye · 2018-02-28T10:40:29Z

src/theme/icon.m.css

-	url('./fonts/dojo2.ttf?fkcsox') format('truetype'),
-	url('./fonts/dojo2.woff?fkcsox') format('woff'),
-	url('./fonts/dojo2.svg?fkcsox#dojo2') format('svg');
+	url('../common/fonts/dojo2.ttf?fkcsox') format('truetype'),


I believe this path was correct before, the font files are in ./fonts

tomdye · 2018-02-28T14:39:58Z

Raised #494 to replace this PR

tomdye requested changes Feb 16, 2018

View reviewed changes

smhigley suggested changes Feb 16, 2018

View reviewed changes

tomdye requested changes Feb 16, 2018

View reviewed changes

smhigley reviewed Feb 16, 2018

View reviewed changes

smhigley approved these changes Feb 16, 2018

View reviewed changes

tomdye requested changes Feb 19, 2018

View reviewed changes

agubler added the breaking-change label Feb 21, 2018

tomdye mentioned this pull request Feb 21, 2018

Rename icons to icon dojo/themes#9

Merged

3 tasks

tomdye approved these changes Feb 27, 2018

View reviewed changes

maier49 force-pushed the icon-widget branch 2 times, most recently from 4c41b05 to 38143d3 Compare February 27, 2018 18:00

maier49 added 14 commits February 27, 2018 13:31

Add icon widget

1b0b261

Icon PR feedback

d3d60b0

Fix file name

0e9dc48

Icon PR feedback

85c5082

Use Icon widget for icons

416df4f

Remove icon css, add onclicks where it makes sense

6f8e79e

Remove onclick and add alt text

52fe9c5

Always hide icon from screen readers

c4ef8fd

Remove newline

2d10d74

Remove new lines

afa7118

Remove extra class

41f57e7

Update README

c20e3fd

Remove extra classes

638d7ed

Properly wrap title pane icon usage

2def11d

maier49 force-pushed the icon-widget branch from 38143d3 to 2def11d Compare February 27, 2018 18:51

maier49 added 3 commits February 27, 2018 13:56

Fix suite name and css import name

c00fc32

Fix button imports

f139f7b

Trigger CI

29a1ad8

tomdye reviewed Feb 28, 2018

View reviewed changes

tomdye requested changes Feb 28, 2018

View reviewed changes

tomdye mentioned this pull request Feb 28, 2018

Icon widget #494

Merged

3 tasks

tomdye closed this Feb 28, 2018

dylans added this to the 2018.02 milestone Feb 28, 2018

Add icon widget #477

Add icon widget #477

Conversation

maier49 commented Feb 16, 2018

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 Feb 16, 2018 • edited Loading

Codecov Report

smhigley 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

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

smhigley left a comment

Choose a reason for hiding this comment

tomdye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdye commented Feb 21, 2018

tomdye commented Feb 21, 2018

tomdye commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdye commented Feb 28, 2018

codecov bot commented Feb 16, 2018 •

edited

Loading