Skip to content

Add imageErrorAs to icon#4009

Merged
dzearing merged 9 commits intomicrosoft:masterfrom
staylo8:staylo/AddErrorAsToIcon
Feb 22, 2018
Merged

Add imageErrorAs to icon#4009
dzearing merged 9 commits intomicrosoft:masterfrom
staylo8:staylo/AddErrorAsToIcon

Conversation

@staylo8
Copy link
Copy Markdown
Collaborator

@staylo8 staylo8 commented Feb 16, 2018

Pull request checklist

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

Description of changes

  • Make Icon statefull
  • Add imageErrorAs: (ImageProps) => any to IIconPros
  • If rendering an image, image has failed to load, and imageErrorAs was provided, render imageErrorAs
  • A few files were impacted by making Icon statefull
  • Also did some cleanup around the aria-label and other container decorations.

@staylo8 staylo8 requested a review from dzearing February 16, 2018 22:57
@dzearing dzearing self-assigned this Feb 17, 2018
className: 'ms-Check-check ' + styles.check,
iconName: 'StatusCircleCheckmark'
}) }
{ <Icon
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The original intent here was that because Check is a hotspot, calling the stateless component directly was a performance optimization. I think the perf optimization is dated though, so I think this is fine.

I made a codepen to test the theory about if JSX or function calls are faster. They look pretty comparable.

https://codepen.io/dzearing/pen/xYpjrQ

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's for creation, is there also a management overhead later when removing etc?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The example also has a "clear" button which also measures, and I didn't see a noticeable impact.

@dzearing
Copy link
Copy Markdown
Member

dzearing commented Feb 18, 2018

/home/travis/build/OfficeDev/office-ui-fabric-react/packages/office-ui-fabric-react/src/components/Button/BaseButton.tsx:257:14
ERROR: 257:14 jsx-wrap-multiline Multiline JSX elements must be wrapped in parentheses
ERROR: 260:9 semicolon Missing semicolon
/home/travis/build/OfficeDev/office-ui-fabric-react/packages/office-ui-fabric-react/src/components/Icon/Icon.tsx:24:6
ERROR: 24:6 semicolon Missing semicolon
ERROR: 31:15 triple-equals == should be ===
ERROR: 36:3 member-access The class method 'render' must be marked either 'private', 'public', or 'protected'
ERROR: 36:3 member-ordering Declaration of public instance method not allowed after declaration of private instance method. Instead, this should come after public constructors.
ERROR: 52:6 semicolon Missing semicolon
ERROR: 62:106 semicolon Missing semicolon
ERROR: 113:2 no-unnecessary-semicolons unnecessary semi-colon
ERROR: 113:2 semicolon Unnecessary semicolon

Make sure to run npm run build from the fabric package directory, you'll see the tslint failures.

classNames.imageContainer,
className
);
export class Icon extends React.Component<IIconProps, IIconState> {
Copy link
Copy Markdown
Member

@dzearing dzearing Feb 20, 2018

Choose a reason for hiding this comment

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

This probably should BaseComponent

/**
* If rendering an image icon, this function callback will be invoked in the event loading the image errors.
*/
imageErrorAs?: (image: IImageProps) => any;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead of any, can you make this type safe? Let me see;

 React.StatelessComponent<IImageProps> | React.ComponentClass<IImageProps>

I think that's it?

@dzearing dzearing merged commit af4a297 into microsoft:master Feb 22, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants