Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add hypertext component #210

Merged
merged 27 commits into from
Apr 13, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
f063d64
rebased
marjonlynch Apr 5, 2018
a129d6a
renamed all to hypertext from hyperlink
marjonlynch Apr 2, 2018
0d262f5
renamed hyperlink to hypertext
marjonlynch Apr 2, 2018
209f428
microsoft appropriate test link used
marjonlynch Apr 2, 2018
ab94569
added hypertext references in react-msft and styles-msft
marjonlynch Apr 4, 2018
5037d28
added md file for hypertext
marjonlynch Apr 4, 2018
99731f6
missed hyperlink reference, and updated md doc to use hypertext
marjonlynch Apr 4, 2018
bdced4c
added more style to hypertext, fixed some possible build issues
marjonlynch Apr 4, 2018
c4da12a
chris helped added a couple good style things, fixed example text
marjonlynch Apr 5, 2018
ad63c30
addressing PR comments
marjonlynch Apr 5, 2018
5c4b7f3
changes to account for testing examples update
marjonlynch Apr 5, 2018
44ef96f
applied more PR comments
marjonlynch Apr 6, 2018
95f2f40
removed the text property on hypertext, will only use children
marjonlynch Apr 6, 2018
f274dfc
added newline to file so it builds?
marjonlynch Apr 6, 2018
c825fdd
updates to fix server build rules
marjonlynch Apr 6, 2018
3e0f94c
more tslint errors fixed
marjonlynch Apr 6, 2018
c5cfa5a
more tslint errors fixed
marjonlynch Apr 6, 2018
3f1e52d
updated snapshots
marjonlynch Apr 6, 2018
dad7b10
refactoring based on PR comments
marjonlynch Apr 6, 2018
451c9ad
merge issue resolved
marjonlynch Apr 6, 2018
148ba95
tslint stuff again
marjonlynch Apr 6, 2018
5b28c9d
PR comments applied
marjonlynch Apr 13, 2018
79d3549
rebased again
marjonlynch Apr 13, 2018
1c68f4d
PR comments applied
marjonlynch Apr 13, 2018
bd699e9
spacing on export fixed
marjonlynch Apr 13, 2018
4ca8ea6
added comments to handled props for hypertext
marjonlynch Apr 13, 2018
2159271
changes to accomodate contract interface update
marjonlynch Apr 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
more tslint errors fixed
  • Loading branch information
marjonlynch committed Apr 13, 2018
commit 3e0f94cf1a85136293822f422779c814b7b26d7c
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ const examples: ISnapshotTestSuite<IHypertextHandledProps & IHypertextManagedCla
children: "MSDN"
}
]
}
};

export default examples;
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ export interface IHypertextHandledProps {
children?: React.ReactNode | React.ReactNode[];
}


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

export interface IHypertextManagedClasses extends IManagedClasses<IHypertextClassNameContract> {}
export type HypertextProps = IHypertextHandledProps & IHypertextUnhandledProps & IHypertextManagedClasses;
19 changes: 9 additions & 10 deletions packages/fast-components-react-base/src/hypertext/hypertext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,6 @@ class Hypertext extends Foundation<IHypertextHandledProps & IManagedClasses<IHyp
managedClasses: void 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, done and done

};

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

/**
* Renders the component
*/
Expand All @@ -32,14 +25,20 @@ class Hypertext extends Foundation<IHypertextHandledProps & IManagedClasses<IHyp
);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it in, learning about lodash today

}

private generateAttributes(): {} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

const attributes = {};
const HREF_INDEX = "href";
const HREF_INDEX: string = "href";

if (this.props.href) {
attributes[HREF_INDEX] = this.props.href;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

}
return attributes;
}
}
Expand Down