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.
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(icons): add @fluentui/react-icons package #12608
feat(icons): add @fluentui/react-icons package #12608
Changes from 3 commits
5c75f0f
4bb74f8
c1ead16
00d92e6
9067043
9d7caee
c755849
5ad91c5
33ef673
9b54c23
ac4d37f
4f29fa9
4c1a26e
b00e1bb
c53857c
c7ad0cf
cd4cd22
8a82319
bb34b65
f90fd23
d02ae3f
8379702
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we discussed whether we want to use default or named exports in new code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, I just used whatever I was used to :S Let me know what makes more sense and will update it..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original thinking with not exporting default was:
A downstream index.ts file might do something like:
Which would not include the default export, but only named ones. That said, we have seen concerns multiple times about
export *
. And there is NO friction inexporting { default as Foo }
explicitly; explicit exports are better anyway in that we don't accidentally leak something unintentionally.So my 2c is "it's fine".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
classNames
get injected on the root?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createSvgIcon
is doing that. I have two alternatives of the factory once that is for injecting only the svg and another one that allows user to have custom props and define the root element too. Let's see if we will need the both or choose oneThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
className="thing", also
classNames
doesn't seem to be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this className is being used in an example, but this doesn't seem like a good pattern to demonstrate in general. Maybe find another way to re-style the element for the example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just copied this examples from the
Icon.Svg.Example.tsx
. Will create new example with showcase of how the things inside the svgs can be styledThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored and improved examples. Please take a look.