feat: add hypertext component #210

marjonlynch · 2018-04-04T21:52:14Z

Adding hypertext component based on the existing button component.

eljefe223 · 2018-04-04T22:01:21Z

packages/fast-components-react-base/src/hypertext/DOCS.md

@@ -0,0 +1,14 @@
+## Best practices
+Use *hyperlink* when you need to navigate the user to more information about the text that was selected. *Hyperlinks* are best used for tertiary actions outside the main work flow.


got it, thanks

chrisdholt · 2018-04-04T22:37:52Z

packages/fast-components-react-base/src/hypertext/DOCS.md

+Use *hypertext* when you need to navigate the user to more information about the text that was selected. *Hypertexts* are best used for tertiary actions outside the main work flow.
+
+### Usage guidance
+*Hypertexts* may appear as standalone elements, on a line by themselves, or inline elements, in the middle of regular text. Use inline *hypertext* within the context of a sentence or a paragraph. Within a paragraph, *hypertext* should inherit the same formatting as the neighboring text as far as font size and font weight. Inline links will appear underlined.


I think we'll want to change our guidance to use these inline only

chrisdholt · 2018-04-04T22:38:31Z

packages/fast-components-react-base/src/hypertext/hypertext.props.ts

@@ -0,0 +1,23 @@
+import * as React from "react";
+
+/**


we can pull this enum as we aren't supporting non-anchor elements and this isn't configurable

chrisdholt · 2018-04-04T22:39:08Z

packages/fast-components-react-base/src/hypertext/hypertext.props.ts

+
+export interface IHypertextProps {
+
+    id?: string;


I'm not sure this needs to be a first class prop for this component. I think we can pull this

chrisdholt · 2018-04-04T22:39:48Z

packages/fast-components-react-base/src/hypertext/hypertext.props.ts

+
+    id?: string;
+
+    text?: string;


Since we're passing children here, text is unnecessary unless we are going to draw a clear distinction

chrisdholt · 2018-04-04T22:42:47Z

packages/fast-components-react-base/src/hypertext/hypertext.props.ts

+
+    href?: string;
+
+    ariaLabel?: string;


based on our conversation yesterday with Mat, I think this is the exception here and likely wouldn't expect this as a first-class prop

chrisdholt · 2018-04-04T22:55:27Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+}
+
+export default Hypertext;
+export {IHypertextProps, IHypertextClassNameContract, HypertextHTMLTags};


you can pull this as it won't exist

chrisdholt · 2018-04-04T22:56:00Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+    };
+
+    protected defaultProps: IHypertextProps = {
+        ariaLabel: void(0),


looks like there are no default props. These would go within handled I believe.

chrisdholt · 2018-04-04T22:56:24Z

packages/fast-components-react-base/src/hypertext/index.ts

+import Hypertext from "./hypertext";
+
+export default Hypertext;
+export * from "./hypertext";


I think we need a newline here

Are we saying that we should have new lines before export statements? If so, should this be a new rule on tslint package?

I sort of liked grouping by component name.

There should be a .ts check for newline at the end of the file. I'm trying to hook up ordering alphabetically in tslint for these import/exports.

chrisdholt · 2018-04-04T22:56:45Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+        let attributes = {};
+
+        if (this.props.href) {
+            attributes['href'] = this.props.href;


these should be double quotes

chrisdholt · 2018-04-04T22:58:42Z

packages/fast-components-styles-msft/src/hypertext/index.ts

+import {ComponentStyles} from "@microsoft/fast-jss-manager";
+import {IHypertextClassNameContract} from "@microsoft/fast-components-class-name-contracts";
+
+const styles: ComponentStyles<IHypertextClassNameContract, IDesignSystem> = {


let me know if you, kham, and Jeff want to jump on a session to do one of these.

awentzel · 2018-04-05T20:52:04Z

packages/fast-components-react-base/src/hypertext/index.ts

+import Hypertext from "./hypertext";
+
+export default Hypertext;
+export * from "./hypertext";


Are we saying that we should have new lines before export statements? If so, should this be a new rule on tslint package?

awentzel · 2018-04-05T20:55:46Z

packages/fast-components-react-base/src/hypertext/index.ts

+import Hypertext from "./hypertext";
+
+export default Hypertext;
+export * from "./hypertext";


I sort of liked grouping by component name.

awentzel · 2018-04-05T20:56:35Z

packages/fast-components-react-msft/src/hypertext/examples.data.ts

@@ -0,0 +1,16 @@
+import {ICategoryItemProps} from "@microsoft/fast-development-site-react";
+import {IGenericExample} from "../examples";


I think you'll want to do a rebase and replace IGenericExample with ISnapshots.... Little bit of a refactor, unfortunately.

nicholasrice · 2018-04-05T19:04:44Z

packages/fast-components-react-base/src/hypertext/hypertext.props.ts

@@ -0,0 +1,10 @@
+import * as React from "react";
+
+export interface IHypertextProps {


We've changed the naming convention slightly for props objects - can you align to button.props.ts?

nicholasrice · 2018-04-05T21:06:38Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+        managedClasses: void 0,
+    };
+
+    private generateAttributes() {


You'll see this approach to applying attributes a lot in the previous repo - the reason we did it this way is we didn't have accurate typescript typings for components.

In this project, we made sure to set things up more accurately - you can just add href={this.props.href} in the render method itself and remove this method - that will reduce the code clutter of having an extra method

@nicholasrice question - if a disabled href is just an anchor without an href, do we want to check for it like this rather than passing straight into the render function?

chrisdholt · 2018-04-06T17:59:13Z

packages/fast-components-react-base/src/hypertext/hypertext.props.ts

+}
+
+
+export interface IHypertextUnhandledProps extends React.AllHTMLAttributes<HTMLElement> {}


I don't think we want all html attributes here as this is just an anchor. We likely want:
React.HTMLAnchorAttributes<HTMLAnchorElement>

chrisdholt · 2018-04-06T18:00:29Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+import {IHypertextClassNameContract, IManagedClasses} from "@microsoft/fast-components-class-name-contracts";
+
+/* tslint:disable-next-line */
+class Hypertext extends Foundation<IHypertextHandledProps & IManagedClasses<IHypertextClassNameContract>,  React.AllHTMLAttributes<HTMLAnchorElement>, {}> {


Instead of AllHTMLAttributes we'll want HTMLAnchorAttributes

Also done? probably

chrisdholt · 2018-04-06T18:01:12Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+/* tslint:disable-next-line */
+class Hypertext extends Foundation<IHypertextHandledProps & IManagedClasses<IHypertextClassNameContract>,  React.AllHTMLAttributes<HTMLAnchorElement>, {}> {
+    protected handledProps: HandledProps<IHypertextHandledProps & IManagedClasses<IHypertextClassNameContract>> = {
+        managedClasses: void 0,


We'll need to enumerate any of the props here that are handled.

I think, done and done

chrisdholt · 2018-04-06T18:01:57Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+        const HREF_INDEX = "href";
+
+        if (this.props.href) {
+            attributes[HREF_INDEX] = this.props.href;


does dot notation not work here? I know that you're likely getting an error, but if we can access via dot notation, let's do that instead.

awentzel

You'll also want to resolve your Travis CI build errors (lint errors). https://travis-ci.org/Microsoft/fast-dna/builds/363248660?utm_source=github_status&utm_medium=notification

janechu · 2018-04-07T01:17:24Z

You'll want to check the body of the PR message and use something like "Closes #181" this will automatically link your PR to the open ticket and close it when the PR is merged in.

nicholasrice · 2018-04-09T17:14:50Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+        return super.generateClassNames(this.props.managedClasses.hypertext);
+    }
+
+    private generateAttributes(): {} {


I think we should remove this function. Because we're not manipulating this data, it can be safely applied with the this.unhandledProps() call.

nicholasrice · 2018-04-09T17:19:23Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+class Hypertext extends Foundation<IHypertextHandledProps & IManagedClasses<IHypertextClassNameContract>, React.AnchorHTMLAttributes<HTMLAnchorElement>, {}> {
+    protected handledProps: HandledProps<IHypertextHandledProps & IManagedClasses<IHypertextClassNameContract>> = {
+        managedClasses: void 0,
+        href: void 0,


We really don't need to enumerate href as a prop either - it is included by the React.AnchorHTMLAttributes interface accurately for this component

nalogiudice

A few JSS nits

nalogiudice · 2018-04-09T21:28:25Z

packages/fast-components-styles-msft/src/hypertext/index.ts

+const styles: ComponentStyles<IHypertextClassNameContract, IDesignSystem> = {
+    hypertext: {
+        borderBottom: (config: IDesignSystem): string => {
+            return `1px solid ${config.brandColor}`;


should use ${toPx(1)} here too

nalogiudice · 2018-04-09T21:28:46Z

packages/fast-components-styles-msft/src/hypertext/index.ts

+        textDecoration: "none",
+        "&:hover, &:focus": {
+            borderBottom: (config: IDesignSystem): string => {
+                return `2px solid ${config.brandColor}`;


Should use ${toPx(2)} here as well

nalogiudice · 2018-04-09T21:30:01Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+        return super.generateClassNames(this.props.managedClasses.hypertext);
+    }
+
+    private generateAttributes(): {} {


chrisdholt · 2018-04-13T19:15:34Z

packages/fast-components-react-base/src/hypertext/hypertext.props.ts

+import { IHypertextClassNameContract, IManagedClasses } from "@microsoft/fast-components-class-name-contracts";
+
+export interface IHypertextHandledProps {
+


can you add comments for the prop definitions

chrisdholt · 2018-04-13T19:17:21Z

packages/fast-components-react-msft/src/hypertext/examples.data.ts

+import * as React from "react";
+
+export default {
+    name: "hypertext",


can you add an example that does not include an href?

chrisdholt · 2018-04-13T19:18:04Z

packages/fast-components-styles-msft/src/index.ts

@@ -4,6 +4,9 @@
 import ButtonStyles from "./button";
 export { ButtonStyles };

+import HypertextStyles from "./hypertext";
+export {HypertextStyles};


I know it's not barking from a tslint standpoint, but can you follow the convention of including spaces on export statements?

yep, I knew about that just missed this one. Doh!

chrisdholt · 2018-04-13T19:19:40Z

packages/fast-components-react-base/src/hypertext/hypertext.tsx

+     * Generates class names
+     */
+    protected generateClassNames(): string {
+        return super.generateClassNames(this.props.managedClasses.hypertext);


this is new within the last day, but we'll want to use lodash's "get" method to access here. It will check that this prop actually exists.

@import { get } from "lodash-es";

super.generateClassNames(get(this.props, "managedClasses.hypertext"));

got it in, learning about lodash today

chrisdholt · 2018-04-13T20:14:36Z

packages/fast-components-class-name-contracts/src/index.ts

@@ -1,6 +1,7 @@
 export * from "./button";
 export * from "./divider";
 export * from "./image";
+export * from "./hypertext";


I mean - we can correct this later if need be, but technically "h" would be before "i" :)

chrisdholt · 2018-04-13T20:14:57Z

packages/fast-components-react-base/app/examples.ts

@@ -12,3 +12,6 @@ export { ToggleExamples };

 import TypographyExamples from "../src/typography/examples.data";
 export { TypographyExamples };
+
+import HypertextExamples from "../src/hypertext/examples.data";
+export {HypertextExamples};


nit: spacing :)

all comments addressed

chrisdholt · 2018-04-13T20:35:18Z

packages/fast-components-react-base/src/hypertext/hypertext.props.ts

+
+export interface IHypertextHandledProps {
+
+    href?: string;


@marjonlynch - maybe it isn't showing the changes...but did you add comments here?

eljefe223 reviewed Apr 4, 2018

View reviewed changes

chrisdholt requested changes Apr 4, 2018

View reviewed changes

awentzel changed the title ~~Adding hypertext component, minimal styles and md file~~ feat: add hypertext component Apr 5, 2018

awentzel added feature A new feature type : feat labels Apr 5, 2018

awentzel requested changes Apr 5, 2018

View reviewed changes

nicholasrice previously approved these changes Apr 5, 2018

View reviewed changes

awentzel assigned marjonlynch Apr 5, 2018

marjonlynch dismissed nicholasrice’s stale review via 735f70f April 5, 2018 22:10

marjonlynch force-pushed the users/marjon/add-hyperlink branch from d5fc108 to 735f70f Compare April 5, 2018 22:10

chrisdholt requested changes Apr 6, 2018

View reviewed changes

awentzel previously requested changes Apr 6, 2018

View reviewed changes

marjonlynch force-pushed the users/marjon/add-hyperlink branch from 6bb7bf6 to 7543ec5 Compare April 6, 2018 21:09

nicholasrice previously requested changes Apr 9, 2018

View reviewed changes

nalogiudice previously requested changes Apr 9, 2018

View reviewed changes

marjonlynch force-pushed the users/marjon/add-hyperlink branch from 578618e to 766e84b Compare April 13, 2018 18:55

chrisdholt previously requested changes Apr 13, 2018

View reviewed changes

chrisdholt reviewed Apr 13, 2018

View reviewed changes

marjonlynch force-pushed the users/marjon/add-hyperlink branch from 17f9dd8 to c5a16c6 Compare April 13, 2018 20:25

marjonlynch added 9 commits April 13, 2018 13:47

rebased

f063d64

renamed all to hypertext from hyperlink

a129d6a

renamed hyperlink to hypertext

0d262f5

microsoft appropriate test link used

209f428

added hypertext references in react-msft and styles-msft

ab94569

added md file for hypertext

5037d28

missed hyperlink reference, and updated md doc to use hypertext

99731f6

added more style to hypertext, fixed some possible build issues

bdced4c

chris helped added a couple good style things, fixed example text

c4da12a

marjonlynch added 18 commits April 13, 2018 13:47

addressing PR comments

ad63c30

changes to account for testing examples update

5c4b7f3

applied more PR comments

44ef96f

removed the text property on hypertext, will only use children

95f2f40

added newline to file so it builds?

f274dfc

updates to fix server build rules

c825fdd

more tslint errors fixed

3e0f94c

more tslint errors fixed

c5cfa5a

updated snapshots

3f1e52d

refactoring based on PR comments

dad7b10

merge issue resolved

451c9ad

tslint stuff again

148ba95

PR comments applied

5b28c9d

rebased again

79d3549

PR comments applied

1c68f4d

spacing on export fixed

bd699e9

added comments to handled props for hypertext

4ca8ea6

changes to accomodate contract interface update

2159271

marjonlynch force-pushed the users/marjon/add-hyperlink branch from c5a16c6 to 2159271 Compare April 13, 2018 21:36

nicholasrice approved these changes Apr 13, 2018

View reviewed changes

chrisdholt approved these changes Apr 13, 2018

View reviewed changes

chrisdholt merged commit 9e363ff into master Apr 13, 2018

marjonlynch deleted the users/marjon/add-hyperlink branch April 16, 2018 16:52

chrisdholt mentioned this pull request Apr 23, 2018

Add Hyperlink component #181

Closed

chrisdholt added pr : feature and removed pr : feature labels May 3, 2018

		@@ -0,0 +1,14 @@
		## Best practices
		Use hyperlink when you need to navigate the user to more information about the text that was selected. Hyperlinks are best used for tertiary actions outside the main work flow.

		@@ -0,0 +1,16 @@
		import {ICategoryItemProps} from "@microsoft/fast-development-site-react";
		import {IGenericExample} from "../examples";

		@@ -0,0 +1,10 @@
		import * as React from "react";

		export interface IHypertextProps {

		}


		export interface IHypertextUnhandledProps extends React.AllHTMLAttributes<HTMLElement> {}

		import { IHypertextClassNameContract, IManagedClasses } from "@microsoft/fast-components-class-name-contracts";

		export interface IHypertextHandledProps {

feat: add hypertext component #210

feat: add hypertext component #210

Conversation

marjonlynch commented Apr 4, 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

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

nicholasrice Apr 5, 2018 • edited Loading

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

awentzel left a comment • edited by marjonlynch Loading

Choose a reason for hiding this comment

janechu commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalogiudice 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

chrisdholt Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholasrice Apr 5, 2018 •

edited

Loading

awentzel left a comment •

edited by marjonlynch

Loading

chrisdholt Apr 13, 2018 •

edited

Loading