Expose FileTypeIcons as HTML strings #15405

arkogupta · 2020-10-07T16:03:51Z

Pull request checklist

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

Description of changes

Rather than exposing the src of the img elements, we can just clone the whole JSX element as a string. If in the future, the implementation changes from <img> to something else, then the corresponding DOM element which will be used can again be stored as a string in the same dictionary, so that the consumer can continue to consume it seamlessly.

Focus areas to test

(optional)

codesandbox-ci · 2020-10-07T16:30:07Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 78b40ff:

Sandbox	Source
Fluent UI Button	Configuration
codesandbox-react-template	Configuration

msft-github-bot · 2020-10-07T16:33:23Z

Perf Analysis

No significant results to display.

All results

Scenario	Render type	7.0 Ticks	PR Ticks	Iterations
BaseButton	mount	894	826	5000
Breadcrumb	mount	42004	42134	5000
Checkbox	mount	1497	1497	5000
CheckboxBase	mount	1218	1267	5000
ChoiceGroup	mount	4828	4787	5000
ComboBox	mount	885	874	1000
CommandBar	mount	7503	7556	1000
ContextualMenu	mount	13415	14136	1000
DefaultButton	mount	1124	1109	5000
DetailsRow	mount	3465	3467	5000
DetailsRowFast	mount	3534	3451	5000
DetailsRowNoStyles	mount	3313	3320	5000
Dialog	mount	1485	1438	1000
DocumentCardTitle	mount	1833	1820	1000
Dropdown	mount	2459	2482	5000
FocusTrapZone	mount	1632	1669	5000
FocusZone	mount	1830	1826	5000
IconButton	mount	1732	1718	5000
Label	mount	317	319	5000
Layer	mount	1923	1905	5000
Link	mount	445	426	5000
MenuButton	mount	1491	1431	5000
MessageBar	mount	2007	2002	5000
Nav	mount	3189	3179	1000
OverflowSet	mount	1377	1350	5000
Panel	mount	1445	1384	1000
Persona	mount	814	810	1000
Pivot	mount	1439	1405	1000
PrimaryButton	mount	1242	1246	5000
Rating	mount	7434	7386	5000
SearchBox	mount	1228	1226	5000
Shimmer	mount	2523	2468	5000
Slider	mount	1429	1463	5000
SpinButton	mount	4995	4892	5000
Spinner	mount	415	394	5000
SplitButton	mount	3091	3086	5000
Stack	mount	478	487	5000
StackWithIntrinsicChildren	mount	1481	1484	5000
StackWithTextChildren	mount	4583	4541	5000
SwatchColorPicker	mount	9926	9977	5000
TagPicker	mount	2674	2708	5000
TeachingBubble	mount	50344	50168	5000
Text	mount	421	404	5000
TextField	mount	1338	1295	5000
Toggle	mount	792	787	5000
button	mount	110	95	5000

size-auditor · 2020-10-07T16:38:47Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 52f5d891a274c6ea84aa4bb14cd961c1b89b2cf0 (build)

Ridvanovskyy · 2020-10-12T11:15:52Z

packages/file-type-icons/src/initializeFileTypeIcons.tsx

+
+    src = `${baseUrlSizeType}.png`;
+    iconName = type + size + PNG_SUFFIX;
+    fileTypeIcons[iconName] = <img src={src} alt="" />;


minor question. Does this alt do something? Looks like it doesn't play any role here.

May be wrong, but I believe this is due to accessibility scanners which will warn when you have an image without considering the alt tag (even providing a blank one is considered "accessible")...

Might not be a bad idea to populate this alt text with type + " icon" ~ the type is not always super descriptive but we try our best to make it so.

That is a good suggestion! Will take that up as a separate PR. Thanks!

dzearing · 2020-10-15T17:46:57Z

packages/file-type-icons/src/initializeFileTypeIcons.tsx

+    src = `${baseUrlSizeType}.svg`;
+    iconName = type + size + SVG_SUFFIX;
+    fileTypeIcons[iconName] = <img src={src} alt="" />;
+    _fileTypeIconsAsString[iconName] = `<img src="${src}" alt="" />`;


Rather than defining this on every definition, why not just generate it on demand in the exported functiongetFileTypeIconAsHTMLString

This icon map is quite large in memory so it would be nice if made on-demand rather than bloat the page more.

arkogupta · 2020-10-19T18:18:52Z

packages/file-type-icons/src/initializeFileTypeIcons.tsx

+//     }
+//   }
+// }
+


Hi @dzearing @KevinTCoughlin
Please have a look at this iteration. I had to do some refactoring and export a couple of functions from getFileTypeIconProps to be able to implement the function getFileTypeIconAsHTMLString which is basically kind of a sister clone of getFileTypeIconProps in that it accepts the same kind of object but rather than just returning the iconName, it returns the entire DOM element as a string.

The other option I had considered is this commented snippet; in which the consumer had to call getFileTypeIconProps to get the iconName and then just pass the iconName to the getFileTypeIconAsHTMLString function, but that IMO required some regEx parsing, and hence I avoided this approach.

can you remove the commented code

@dzearing , removed.

khmakoto · 2020-10-24T00:26:58Z

Now that we're publishing beta versions of 8.0 please also port this to the master branch once it merges.

arkogupta · 2020-11-02T17:14:08Z

@KevinTCoughlin @dzearing please have a look whenever you get time. Thanks :)

…upta/office-ui-fabric-react into arkgupta/fileTypeIconsHTML

change/@uifabric-file-type-icons-2020-10-07-21-27-44-arkgupta-fileTypeIconsHTML.json

…fileTypeIconsHTML.json

dzearing · 2020-11-05T15:14:38Z

packages/file-type-icons/src/getFileTypeIconAsHTMLString.ts

+  IFileTypeIconOptions,
+} from './getFileTypeIconProps';
+
+export function getFileTypeIconAsHTMLString(


Can you please add a comment to this? Also the function is not being exported through the index file. And you will probably need to run yarn update-api to update the api-extractor definition of the library.

Added a comment and exported through the index file. I couldn't find an existing api.md or api-extractor.json file for this package. Should I add one?

@dzearing gentle ping! :)

We should add an API extractor config and API file for this package (same pattern as other packages).

dzearing · 2020-11-05T15:16:06Z

packages/file-type-icons/src/initializeFileTypeIcons.tsx

@@ -5,8 +5,8 @@ import { FileTypeIconMap } from './FileTypeIconMap';
 const PNG_SUFFIX = '_png';
 const SVG_SUFFIX = '_svg';

-const DEFAULT_BASE_URL = 'https://spoprod-a.akamaihd.net/files/fabric-cdn-prod_20201008.001/assets/item-types/';
-const ICON_SIZES: number[] = [16, 20, 24, 32, 40, 48, 64, 96];
+export const DEFAULT_BASE_URL = 'https://spoprod-a.akamaihd.net/files/fabric-cdn-prod_20201008.001/assets/item-types/';


A little confused why these are being exported.

(and the icon_sizes)

I am importing both of these in my test file. Also I need the default url while emitting the HTML as string from the new API.

…upta/office-ui-fabric-react into arkgupta/fileTypeIconsHTML

dzearing · 2020-11-17T16:07:50Z

I am going to sign off to unblock the scenario; however I do think longer term we need to split out a separate @fluentui/file-type-icons package which is more the base package that has no React dependencies. This is where we'd do something like export a getFileTypeIcon api which returns more agnostic information in a structure.

ecraig12345

Coming in late here (sorry) but a couple comments to potentially address in the future.

ecraig12345 · 2020-11-17T21:05:58Z

change/@uifabric-file-type-icons-2020-10-07-21-27-44-arkgupta-fileTypeIconsHTML.json

@@ -0,0 +1,8 @@
+{
+  "type": "patch",


For future reference, changes adding new public APIs should be minor not patch.

ecraig12345 · 2020-11-17T21:06:42Z

packages/file-type-icons/src/getFileTypeIconAsHTMLString.ts

+  IFileTypeIconOptions,
+} from './getFileTypeIconProps';
+
+export function getFileTypeIconAsHTMLString(


We should add an API extractor config and API file for this package (same pattern as other packages).

msft-github-bot · 2020-11-18T12:26:59Z

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

arkogupta added 3 commits October 7, 2020 21:24

fileicons as html strings

2c21230

Change files

4151530

missing quotes

21ae27a

arkogupta requested review from bigbadcapers and Jahnp as code owners October 7, 2020 16:03

msft-github-bot added Fabric 7 Area: Icons Package: file-type-icons labels Oct 7, 2020

msft-github-bot assigned tomi-msft Oct 7, 2020

msft-github-bot added the needs cherry-pick Temporary label for PRs which may need to be cherry-picked to master label Oct 7, 2020

arkogupta marked this pull request as draft October 7, 2020 16:07

fix src

b728cfd

arkogupta marked this pull request as ready for review October 7, 2020 18:46

khmakoto mentioned this pull request Oct 7, 2020

PRs targeting the 7.0 branch that will need porting into master #15285

Closed

40 tasks

layershifter closed this Oct 8, 2020

layershifter reopened this Oct 8, 2020

arkogupta mentioned this pull request Oct 12, 2020

Expose filetypeIcon src through a utility #15348

Closed

2 tasks

Ridvanovskyy reviewed Oct 12, 2020

View reviewed changes

dzearing reviewed Oct 15, 2020

View reviewed changes

create HTML elements on demand

6a02416

arkogupta commented Oct 19, 2020

View reviewed changes

arkogupta requested review from KevinTCoughlin and dzearing October 19, 2020 18:21

give better types

2e8459c

arkogupta and others added 2 commits October 29, 2020 23:30

change toBe to toEqual for consistency

d2e11ae

Merge branch '7.0' into arkgupta/fileTypeIconsHTML

1dfaaac

arkogupta added 2 commits November 5, 2020 16:32

remove comment

d242039

Merge branch 'arkgupta/fileTypeIconsHTML' of https://github.com/arkog…

3216b30

…upta/office-ui-fabric-react into arkgupta/fileTypeIconsHTML

dzearing reviewed Nov 5, 2020

View reviewed changes

change/@uifabric-file-type-icons-2020-10-07-21-27-44-arkgupta-fileTypeIconsHTML.json Outdated Show resolved Hide resolved

Update change/@uifabric-file-type-icons-2020-10-07-21-27-44-arkgupta-…

2990c01

…fileTypeIconsHTML.json

dzearing reviewed Nov 5, 2020

View reviewed changes

arkogupta added 2 commits November 6, 2020 01:16

addressing review comments

c7d842b

Merge branch 'arkgupta/fileTypeIconsHTML' of https://github.com/arkog…

78b40ff

…upta/office-ui-fabric-react into arkgupta/fileTypeIconsHTML

ecraig12345 closed this Nov 7, 2020

ecraig12345 reopened this Nov 7, 2020

dzearing merged commit fef6e99 into microsoft:7.0 Nov 17, 2020

ecraig12345 reviewed Nov 17, 2020

View reviewed changes

ecraig12345 unassigned tomi-msft Jan 26, 2021

kubkon self-assigned this Jan 29, 2021

kubkon removed their assignment Feb 8, 2021

tomi-msft mentioned this pull request Feb 18, 2021

Cherrypick filetypeicons as html string to master branch #17067

Merged

2 tasks

ecraig12345 removed the needs cherry-pick Temporary label for PRs which may need to be cherry-picked to master label Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose FileTypeIcons as HTML strings #15405

Expose FileTypeIcons as HTML strings #15405

arkogupta commented Oct 7, 2020 •

edited

Loading

codesandbox-ci bot commented Oct 7, 2020 •

edited

Loading

msft-github-bot commented Oct 7, 2020 •

edited

Loading

size-auditor bot commented Oct 7, 2020 •

edited

Loading

Ridvanovskyy Oct 12, 2020

dzearing Oct 15, 2020

bigbadcapers Oct 16, 2020

arkogupta Nov 5, 2020

dzearing Oct 15, 2020

KevinTCoughlin Oct 15, 2020

arkogupta Oct 19, 2020

dzearing Nov 4, 2020

arkogupta Nov 5, 2020 •

edited

Loading

khmakoto commented Oct 24, 2020

arkogupta commented Nov 2, 2020

dzearing Nov 5, 2020

arkogupta Nov 5, 2020

arkogupta Nov 11, 2020

ecraig12345 Nov 17, 2020

dzearing Nov 5, 2020

dzearing Nov 5, 2020

arkogupta Nov 5, 2020

dzearing commented Nov 17, 2020

ecraig12345 left a comment

ecraig12345 Nov 17, 2020

ecraig12345 Nov 17, 2020

msft-github-bot commented Nov 18, 2020

Expose FileTypeIcons as HTML strings #15405

Expose FileTypeIcons as HTML strings #15405

Conversation

arkogupta commented Oct 7, 2020 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

codesandbox-ci bot commented Oct 7, 2020 • edited Loading

msft-github-bot commented Oct 7, 2020 • edited Loading

size-auditor bot commented Oct 7, 2020 • edited Loading

Asset size changes

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

arkogupta Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

khmakoto commented Oct 24, 2020

arkogupta commented Nov 2, 2020

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

dzearing commented Nov 17, 2020

ecraig12345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-github-bot commented Nov 18, 2020

arkogupta commented Oct 7, 2020 •

edited

Loading

codesandbox-ci bot commented Oct 7, 2020 •

edited

Loading

msft-github-bot commented Oct 7, 2020 •

edited

Loading

size-auditor bot commented Oct 7, 2020 •

edited

Loading

arkogupta Nov 5, 2020 •

edited

Loading