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(react-file-type-icons): Changing FileIconType-based icon searching to use the same scalable method used for file extensions #32947

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

bigbadcapers
Copy link
Contributor

@bigbadcapers bigbadcapers commented Sep 30, 2024

Changes:

  • Changing FileIconType-based icon searching to use the same scalable method used for file extensions
  • Code is up-to-date with the master branch
  • Your changes are covered by tests: Yes, tested in react-experiments under the FileTypeIcon storybook.
  • You've run yarn change locally

Change in Behavior

Adding a new non-file-extension-based filetype icon required code change in multiple places. We've changed it so you only need to register the new type in FileIconType.ts and map it to an existing icon image in FileTypeIconMap.ts.

The API and calling pattern has not changed. Overall line code count and complexity have been reduced.

@bigbadcapers bigbadcapers added the Fluent UI react (v8) Issues about @fluentui/react (v8) label Sep 30, 2024
@fabricteam
Copy link
Collaborator

fabricteam commented Sep 30, 2024

📊 Bundle size report

✅ No changes found

@bigbadcapers bigbadcapers marked this pull request as ready for review September 30, 2024 17:28
@bigbadcapers bigbadcapers requested review from Jahnp and a team as code owners September 30, 2024 17:28
@bigbadcapers bigbadcapers marked this pull request as draft September 30, 2024 19:20
… wrapped in an if statement to filter unwanted properties from the prototype guard-for-in
@bigbadcapers bigbadcapers marked this pull request as ready for review September 30, 2024 20:25
@bigbadcapers
Copy link
Contributor Author

Hey @ThomasMichon / @soettl I put this together a few weeks ago, aiming to make future addons more scalable. Would appreciate your take on the change.

@bigbadcapers
Copy link
Contributor Author

hi @dzearing @ecraig12345 @KatherineThayerMicrosoft would appreciate your take on this PR and if you see any issues with this change of method

Copy link

github-actions bot commented Nov 8, 2024

📊 Bundle size report

✅ No changes found

Copy link

github-actions bot commented Nov 8, 2024

Pull request demo site: URL

@ecraig12345
Copy link
Member

I don't work on Fluent any more. I think @tomi-msft owns icons now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fluent UI react (v8) Issues about @fluentui/react (v8)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants