Experimental Chiclet Component by natalieethell · Pull Request #4678 · microsoft/fluentui

natalieethell · 2018-04-25T18:59:45Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

Basic prototype of the medium-sized ChicletCard.

Basic Medium-Sized Chiclet Example

Basic Medium-Sized Chiclet with Breadcrumb Example

KevinTCoughlin · 2018-04-25T19:24:32Z

packages/experiments/src/components/Chiclet/Chiclet.tsx

+      });
+    }
+
+    switch (size) {


Each case returns the same DOM as far as I can tell. Intentional as an area of extensibility later?

Yes exactly!

In general, we should not add extra prototype code that we know will need to change eventually. Let's just add to this when we get there. Maybe add a @todo comment to mention what you intend to do.

KevinTCoughlin · 2018-04-25T19:27:19Z

packages/experiments/src/components/FloatingPicker/Suggestions/Suggestions.tsx

  protected _selectedElement: HTMLDivElement;
  private SuggestionsItemOfProperType: new (props: ISuggestionItemProps<T>) => SuggestionsItem<T> =
-  SuggestionsItem as new (props: ISuggestionItemProps<T>) => SuggestionsItem<T>;
+    SuggestionsItem as new (props: ISuggestionItemProps<T>) => SuggestionsItem<T>;


Intentional format change?

Looks like it was some auto-indentation formatting done by Visual Studio Code. Probably a good change for readability, though

KevinTCoughlin · 2018-04-25T19:30:07Z

packages/experiments/src/components/Chiclet/ChicletCard.tsx

+        height={ imageHeight }
+        src={ TestImages.documentPreview }
+        role='presentation'
+        alt=''


We should try to parse and provide alt text for any images used in this component at some point for users with accessibility needs. Otherwise a person with vision problems using assistive technology won't know what this image is of. It looks like OG offers og:image:alt for this reason.

https://webaim.org/techniques/alttext/#intro

FYI - I use to help find common a11y issues if you find it helpful, http://wave.webaim.org/extension/

KevinTCoughlin · 2018-04-25T19:31:40Z

packages/experiments/src/components/Chiclet/Chiclet.types.ts

+  /**
+   * Chiclet size to render
+   */
+  size?: string;


Can we leverage the enum ChicletType here for type-safety? I know TS is a bit odd with key/value for enums, but it should be do-able.

KevinTCoughlin · 2018-04-25T19:34:09Z

packages/experiments/src/components/Chiclet/Chiclet.tsx

+  private _classNames: IChicletStyles = {};
+
+  public render() {
+    const { styles: customStyles, chicletCardProps, size, actions, theme } = this.props;


Can probably add { ... onClick = _onClick ... } = this.props; and remove it from the manual DOM assignment below. I think we do it across the lib for functions that can be supplied via props including event handlers (tho this link isn't an event handler example): https://github.com/OfficeDev/office-ui-fabric-react/blob/fee79fb9e115dd282a4337418ac77ed4f1c6cad7/packages/office-ui-fabric-react/src/components/TextField/TextField.tsx#L150

KevinTCoughlin · 2018-04-25T19:35:45Z

packages/experiments/src/components/Chiclet/Chiclet.types.ts

+  imageSecureUrl?: string;
+  imageWidth?: string;
+  imageHeight?: string;
+  imageType?: string;


As mentioned below I'd add imageAlt to this list

Might even consider extracting image related meta fields to its own enum, but that might be over-engineering right now.

KevinTCoughlin · 2018-04-25T19:36:59Z

packages/experiments/src/components/Chiclet/Chiclet.types.ts

+}
+
+export enum ChicletType {
+  /**


You likely don't need to supply values for this enum in its current usage. You may need it if you use the values later as part of assigning the sizes to the props for the node. I left a comment elsewhere about that potential enhancement.

KevinTCoughlin · 2018-04-25T19:38:56Z

packages/experiments/src/components/Chiclet/ChicletCard.tsx

+  }
+
+  private _onKeyDown = (ev: React.KeyboardEvent<HTMLElement>): void => {
+    if (ev.which === KeyCodes.enter || ev.which === KeyCodes.space) {


FYI because I fixed a bug elsewhere about this. If the node could potentially be a <button/> then this will fire twice. Once for _onClick and again for _onKeyDown which will in turn invoke _onAction twice. Native onclick fires for SPACE is why this occurs.

Good to know, thanks!

KevinTCoughlin · 2018-04-25T19:40:24Z

packages/experiments/src/components/Chiclet/ChicletCard.tsx

+      onClick(ev);
+    } else if (!onClick && onClickHref) {
+      // If no onClick Function was provided and we do have an onClickHref, redirect to the onClickHref
+      window.location.href = onClickHref;


If I hold CTRL or CMD and click does it open the link in a new tab? That would be expected browser behavior so I'd check that scenario.

Good point! I'll have to change that. For now though, I'm going to wait to see what exactly we're expecting from onClick for Chiclets before implementing further. I'll keep this as a placeholder for now.

I'd say let's keep things simple and remove the onClickHref till we need to...

KevinTCoughlin · 2018-04-25T19:43:15Z

packages/experiments/src/components/Chiclet/Chiclet.types.ts

+  /**
+   * Function to call when the card is clicked or keyboard Enter/Space is pushed.
+   */
+  onClick?: (ev?: React.SyntheticEvent<HTMLElement>) => void;


Might be better to use the type React.MouseEvent which extends SyntheticEvent.

KevinTCoughlin · 2018-04-25T19:45:13Z

packages/experiments/src/components/Chiclet/BaseChiclet.tsx

+  public render() {
+    const { url, size, actions } = this.props;
+
+    let chicletCardProps = this.extractMetaTags(url);


I'm curious to see what others think here being relatively new to React. Do we want to be fetching these attributes on render? And if so is XMLHttpRequest preferred? Leaving a comment so I can follow-up for my own learning.

If we need to fetch, perhaps componentDidMount is better? Honestly need to defer to the rest of the team here.

Good point. I'm also interested to hear what other people think!

+1.Let's add it to componentWillReceiveProps and put the results in state, so we get to recompute if and only if the input url changes

KevinTCoughlin · 2018-04-25T19:45:32Z

packages/experiments/src/components/Chiclet/BaseChiclet.tsx

+    let chicletCardProps = this.extractMetaTags(url);
+
+    return (
+      <Chiclet chicletCardProps={ chicletCardProps } size={ size ? size : "medium" } actions={ actions } />


As mentioned elsewhere can prob use Enum value here for type safety

KevinTCoughlin · 2018-04-25T19:46:39Z

Hope you don't mind the comments. Mostly went thru so I can learn what's all involved in adding a new component 😄

aditima · 2018-05-01T21:49:58Z

packages/experiments/src/components/Chiclet/ChicletCard.types.ts

+  /**
+   * Function to call when the action is clicked.
+   */
+  onClick?: (ev?: any) => void;


+1, no any please :)

aditima · 2018-05-01T21:54:29Z

packages/experiments/src/components/Chiclet/ChicletCard.types.ts

+   * OpenGraph props.
+   */
+  title?: string;
+  ogType?: string;


remove the OG reference here. ChicletCard should not be aware of the source of the inputs.

aditima · 2018-05-01T21:56:37Z

packages/experiments/src/components/Chiclet/Chiclet.types.ts

+  /**
+   * Action icon buttons to render.
+   */
+  actions?: string[];


I still think the type of actions should be IChicletAction all the way up. We'd need to send in the onClick too, right?

aditima · 2018-05-01T22:00:08Z

packages/experiments/src/components/Chiclet/OpenGraph.ts

+import { IChicletCardProps } from './ChicletCard.types';
+
+export class OpenGraphUtilities {
+  public static extractMetaTags(url: string) {


Add the return type.

I'd call this getOpenGraphChicletProps or something similar

aditima · 2018-05-01T22:01:09Z

packages/experiments/src/components/Chiclet/OpenGraph.ts

+    xmlHttp.send(null);
+
+    var metaElements = document.getElementsByTagName("meta");
+    let openGraphObject = this._getOpenGraphValues(metaElements, attributes);


Why do you want to pass in attributes? Can't we just construct it in _getOpenGraphValues and return?

aditima · 2018-05-01T22:01:50Z

packages/experiments/src/components/Chiclet/OpenGraph.ts

@@ -0,0 +1,112 @@
+import { IChicletCardProps } from './ChicletCard.types';
+
+export class OpenGraphUtilities {


I would prefer to export the one static function instead of adding the class wrapper...

aditima · 2018-05-15T22:18:01Z

packages/experiments/src/components/Chiclet/BaseChiclet.base.tsx

+const getClassNames = classNamesFunction<IBaseChicletStyleProps, IBaseChicletStyles>();
+
+@customizable('BaseChicletBase', ['theme'])
+export class BaseChicletBase extends BaseComponent<IBaseChicletProps, any> {


how about we rename

BaseChiclet to Chiclet (I like this because it's the top order Chiclet component exported from Fabric)

Chiclet to SizedChiclet (this is a mid-tier layer, so don't care as much about its name)

aditima · 2018-05-15T22:20:22Z

packages/experiments/src/components/Chiclet/ChicletCard.base.tsx

+          <div
+            className={ mergeStyles(this._classNames.title) }
+          >
+            { title ? title : "Placeholder" }


Do we really want hardcoded english text as the default? if title is an optional prop, we should be able to render a ChicletCard without one.

aditima · 2018-05-15T22:27:06Z

packages/experiments/src/components/Chiclet/Chiclet.types.ts

+  /**
+   * Medium Chiclet
+   */
+  medium = 2,


Most enums in the fabric codebase seem to be lowerCamelCased... let's go with consistency now, and if we want to change the convention, do it all in one go?

JasonGore

Sorry I didn't notice this earlier, but I don't see any tests.. can we add snapshot tests with various configurations? They should ideally give you confidence that someone won't break all of your hard work. 😃

micahgodbolt · 2018-05-17T21:53:07Z

packages/experiments/src/components/Chiclet/Chiclet.types.ts

+  /**
+   * Description to render for the component.
+   */
+  description?: React.ReactElement<any>


most fabric components default to strings for description. is there a specific need for using any element? Makes it a bit less predictable.

Good question! For now, the description here serves as a kind of "location" for the chiclet, so the idea is to be able to use either a url string or a Breadcrumb, which is why I've made it more generic. One example I have with the Breadcrumb is in Chiclet.Breadcrumb.Example.tsx and one with the basic url string is in Chiclet.Basic.Example.tsx.

In the future, depending on who is using the chiclet, users may want to be able to pass in their own "description" here. Perhaps they'll just want text, or maybe something a little richer like the breadcrumb, or maybe something of their own.

@aditima may be able to speak a little more to this idea.

micahgodbolt · 2018-05-17T21:56:36Z

Just realized this was in experiments :D Be sure to add that in the description. We'll be a bit more 'flexible' on the code in that case. I see you did say 'prototype', but I went and made it more obvious.

Is experiemental component

JasonGore · 2018-05-17T22:20:32Z

Sorry, I didn't notice it was experimental either.

dzearing · 2018-05-20T19:32:55Z

Please include screenshots in the description when new components are added :)

dzearing · 2018-05-20T19:33:40Z

/home/travis/build/OfficeDev/office-ui-fabric-react/packages/experiments/src/components/Chiclet/Chiclet.types.ts:43:36
ERROR: 43:36  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 48:31  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
/home/travis/build/OfficeDev/office-ui-fabric-react/packages/experiments/src/components/Chiclet/ChicletCard.types.ts:67:36
ERROR: 67:36  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 72:31  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
/home/travis/build/OfficeDev/office-ui-fabric-react/packages/experiments/src/components/Chiclet/ChicletPage.tsx:55:15
ERROR: 55:15  jsx-self-close         JSX elements with no children must be self-closing
/home/travis/build/OfficeDev/office-ui-fabric-react/packages/experiments/src/components/Chiclet/OpenGraph.ts:4:7
ERROR: 4:7    prefer-const           Identifier 'attributes' is never reassigned; use 'const' instead of 'let'.
ERROR: 45:7   prefer-const           Identifier 'xmlHttp' is never reassigned; use 'const' instead of 'let'.
/home/travis/build/OfficeDev/office-ui-fabric-react/packages/experiments/src/components/Chiclet/examples/Chiclet.Basic.Example.tsx:10:58
ERROR: 10:58  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 10:63  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 19:21  no-use-before-declare  variable 'FooterComponent' used before declaration
ERROR: 27:54  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 27:59  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 44:93  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
/home/travis/build/OfficeDev/office-ui-fabric-react/packages/experiments/src/components/Chiclet/examples/Chiclet.Breadcrumb.Example.tsx:14:63
ERROR: 14:63  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 14:68  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 24:8   no-use-before-declare  variable 'FooterComponent' used before declaration
ERROR: 64:54  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 64:59  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
ERROR: 81:93  no-any                 Type declaration of 'any' loses type-safety. Consider replacing it with a more precise type.
[@uifabric/experiments 19:35:11] Error: tslint (29.334s): Error: Command failed: node /home/travis/build/OfficeDev/office-ui-fabric-react/scripts/node_modules/tslint/lib/tslint-cli --project /home/travis/build/OfficeDev/office-ui-fabric-react/packages/experiments/tsconfig.json -t stylish -r /home/travis/build/OfficeDev/office-ui-fabric-react/common/temp/node_modules/tslint-microsoft-contrib

dzearing · 2018-05-20T19:34:24Z

Make sure to run npm run build tslint in the experiments folder to remove all the lint errors.

Also install the tslint extension to see the errors before you add them.

dzearing · 2018-05-20T19:39:02Z

packages/experiments/src/components/Chiclet/ChicletCard.styles.ts

+        backgroundColor: palette.white,
+        borderRadius: '2px',
+        boxShadow: '0 1px 3px 0 rgba(0, 0, 0, 0.3)',
+        width: '600px',


numerical pixel values in styling can be represented as numbers rather than strings. :)

@cschleiden

* master: (67 commits) Applying package updates. Revert ChoiceGroup change in 5.0 to minimize potential partner impact. (microsoft#4962) Applying package updates. ChoiceGroup: getStyles conversion (microsoft#4852) Export SASS variables and mixins (microsoft#4959) Variants: update algorithm (microsoft#4949) Allow for customization of keycodes that cause the focus rect to appear (microsoft#4948) Mergestyles facepile (microsoft#4950) Fixing circular dependency and non-AMD references in ContextualMenu (microsoft#4946) Clean up semantic slots (microsoft#4932) Added missing merge-styles background-size typing (microsoft#4935) Applying package updates. Split menu button styles (microsoft#4922) Experimental Chiclet Component (microsoft#4678) Remove hover and pressed background colors (microsoft#4908) Remove @cschleiden as Rating code owner (microsoft#4929) Addressing Issue microsoft#3691 - SplitButton: In Safari, the SplitButton has mismatched heights (microsoft#4797) Applying package updates. Adding option for focus on tags in disabled picker (microsoft#4833) Jest merge-styles serialization fix for animation-name (microsoft#4927) ...

natalieethell added 17 commits April 6, 2018 14:31

base chiclet page

fb26b91

moving Chiclet to experiments

ad94997

other changes to old chiclet

542fcf8

basic chiclet moved to experiments

c6f0aab

get rid of chiclet references in old directory

5d7f668

get rid of last Chiclet reference in wrong directory

56f52c9

added icons and started cleaning up

15ccf73

almost fixed hover border

1012e4c

can choose chiclet size to render and cleanup

ee82f29

three layers of the Chiclet

fd42f49

actions can now be passed in as strings

fc90f50

Chiclet -> BaseChiclet, ChicletPicker -> Chiclet

3fa9d9e

ChicletPicker to Chiclet

3df9860

styles sheet working

a2b0fa5

styling moved to styles sheet

2e5978d

remove references to scss file

e497323

remove Chiclet.scss and XSmall

d0b31a2

KevinTCoughlin reviewed Apr 25, 2018

View reviewed changes

aditima approved these changes May 17, 2018

View reviewed changes

add return type to getOpenGraphProperties

6bad6f0

JasonGore previously requested changes May 17, 2018

View reviewed changes

micahgodbolt reviewed May 17, 2018

View reviewed changes

micahgodbolt changed the title ~~Chiclet Component~~ Experimental Chiclet Component May 17, 2018

Merge branch 'master' into chiclet

49c756b

natalieethell added 7 commits May 17, 2018 17:05

fixed errors from npm run build

972d37d

removed unnecessary whitespace change

16156a8

snapshot test

275bbc2

better description for snapshot test

db80bf2

small fix for preview image

70badd3

change preview image link to sharepoint link

5b381fb

change file for Chiclet

8f17013

dzearing reviewed May 20, 2018

View reviewed changes

natalieethell added 7 commits May 21, 2018 10:02

fixed tslint errors and added state to Chiclet

9a7d9e8

small bug fix for image preview and change sample url

be9118f

numerical pixel values represented as numbers rather than strings

b33c5b8

removed unnecessary styling names

51f1ba8

description and footer now JSX.Elements

679e0a6

removed unnecessary styling from Chiclet and updated snapshot

c404200

Merge branch 'master' into chiclet

7a550dc

natalieethell merged commit 841a6de into microsoft:master May 21, 2018

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

		@@ -0,0 +1,112 @@
		import { IChicletCardProps } from './ChicletCard.types';

		export class OpenGraphUtilities {

Conversation

natalieethell commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinTCoughlin Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinTCoughlin Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinTCoughlin commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonGore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

natalieethell commented Apr 25, 2018 •

edited

Loading

KevinTCoughlin Apr 25, 2018 •

edited

Loading

KevinTCoughlin Apr 25, 2018 •

edited

Loading

micahgodbolt commented May 17, 2018 •

edited

Loading