This repository has been archived by the owner on Mar 4, 2020. It is now read-only.
feat: themable icons #260
Merged
feat: themable icons #260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* generalize API for svg and font-based icons * remove 'svg' prop in IconSet example * add mechanism for providing font-based icons in theme * fix UTs * remove getExtraProps method * fix theme icons example in docs * generalize object and function types as icon descriptors * fix unit tests * address review comments
…react into feat/themable-icons
kuzhelov
requested review from
dvdzkwsk,
levithomason,
miroslavstastny and
mnajdova
as code owners
1 task
Codecov Report
@@ Coverage Diff @@
## master #260 +/- ##
==========================================
- Coverage 92.57% 92.19% -0.39%
==========================================
Files 62 61 -1
Lines 1131 1127 -4
Branches 168 147 -21
==========================================
- Hits 1047 1039 -8
- Misses 81 84 +3
- Partials 3 4 +1
Continue to review full report at Codecov.
|
src/lib/renderComponent.tsx
Outdated
| const ElementType = getElementType({ defaultProps }, props) | ||
|
|
||
| const stateAndProps = { ...state, ...props } | ||
| const derivedProps = getExtraProps({ icons }) |
There was a problem hiding this comment.
It looks like you took old changes from #183.
getExtraProps was removed.
There was a problem hiding this comment.
thanks for the heads up - this artefact had leaked in during merge, as there were lots of conflicts with master. Thanks a lot!
kuzhelov
commented
Sep 21, 2018
src/components/Icon/Icon.tsx
Outdated
| @@ -120,22 +114,22 @@ class Icon extends UIComponent<Extendable<IIconProps>, any> { | |||
| return <ElementType className={classes.root} {...accessibility.attributes.root} {...rest} /> | |||
| } | |||
|
|
|||
| renderSvgIcon(ElementType, classes, rest, accessibility): React.ReactNode { | |||
| renderSvgIcon(ElementType, icons: ThemeIcons, classes, rest, accessibility): React.ReactNode { | |||
src/components/Icon/Icon.tsx
Outdated
| renderComponent({ ElementType, classes, rest, accessibility, theme }) { | ||
| const { icons = {} } = theme | ||
|
|
||
| return icons[this.props.name] && icons[this.props.name].isSvg |
There was a problem hiding this comment.
I would prefer to access the array only once, store icons[this.props.name] to a variable and pass it to renderSvgIcon() instead of icons
miroslavstastny
approved these changes
Sep 21, 2018
mnajdova
reviewed
Sep 21, 2018
| @@ -6,6 +6,14 @@ export type Extendable<T> = T & { | |||
| [key: string]: any | |||
| } | |||
|
|
|||
| export type ArgOf<T> = T extends (arg: infer TArg) => any ? TArg : never | |||
| export type ResultOf<T> = T extends (...arg: any[]) => infer TResult ? TResult : never | |||
|
|
|||
mnajdova
approved these changes
Sep 21, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR represents accumulated changes from the following two (both were approved to merge):