Add Custom Icons for Message Bar #10294
Conversation
|
I have no idea why the extra changelist happened, but I set them all to be none except for the office-oinline-ui one |
|
I pulled from master now and now locally I have a lot of snapshot tests that are failing from things I didn't touch like DetailRows and Nav, any idea on how I can resolve this without updating the snapshot? |
There was a problem hiding this comment.
Not sure what caused the issue with the extra changelist is. Be sure to get to a clean state before running yarn change & yarn update-snapshots. If you hadn't touched this repo for a while, it has been converted to use yarn. If all else fails, there's this new util @ecraig12345 made called yarn scrub that would nuke everything (not checked in, and not part of our repo).
| return ( | ||
| <div className={this._classNames.iconContainer} aria-hidden> | ||
| <Icon iconName={this.ICON_MAP[this.props.messageBarType!]} className={this._classNames.icon} /> | ||
| <Icon iconName={messageBarIcon ? messageBarIcon : this.ICON_MAP[this.props.messageBarType!]} className={this._classNames.icon} /> |
There was a problem hiding this comment.
Seems like this component already has some kind of mechanism to specify icons based on this map. The only problem is that this map is a private const. Perhaps what you should do is add a more generic prop that allows customization of the icon map (iconMap or something). It could override the defaults where appropriate?
@ecraig12345 - looks like you're the code owner. Mind if you chimed in here?
There was a problem hiding this comment.
I think I actually prefer keeping the separate prop because it keeps the MessageBar code simpler while still handling the most common cases (and if the consumer wants to pass a different custom icon based on the MessageBarType, they can handle that in their code).
| @@ -81,6 +81,18 @@ export interface IMessageBarProps extends React.HTMLAttributes<HTMLElement> { | |||
| * Call to provide customized styling that will layer on top of the variant rules. | |||
| */ | |||
| styles?: IStyleFunctionOrObject<IMessageBarStyleProps, IMessageBarStyles>; | |||
|
|
|||
| /** | |||
| * Custom icon to replace the dismiss icon. | |||
There was a problem hiding this comment.
Do you have a specific scenario where you need to change the dismiss icon to something besides an X? I can see why you might need to change the main icon, but using something besides X to close is a bit odd.
There was a problem hiding this comment.
The specific scenario (and the real reason why I made this change) is that my specific project has the X icons of varying sizes, as a result we don't call our X button "Clear", we would call it something like Clear_16 as an example.
There was a problem hiding this comment.
Interesting, can you not just change the font size with styling? (styles.dismissal)
There was a problem hiding this comment.
My problem is less of the styles/size of the dismiss button, but more that I'm forced to use a specific icon name for the dismiss button.
That and in our own project Clear is defined to be a different icon, so I would like to not interweave our icon names
There was a problem hiding this comment.
Interesting. I'm not so enthusiastic about allowing this because it implies that we should do the same on all the other components with close buttons (not going to ask you to do that, but it opens the possibility), and I'm concerned that it sort of invites bad UX patterns (like using something besides an x for a close button). So I'll have to talk to some people about this tomorrow and get back to you.
There was a problem hiding this comment.
Sounds good! My only argument against what we do today is that the component has a dependency on a specific close icon that no one would know about unless they look through the code or happen to map all of Fabric's icon.
I don't actually know much about icon mapping process, but I assume its some extra steps that needs to be taken by the consumer.
packages/office-ui-fabric-react/src/components/MessageBar/MessageBar.types.ts
Outdated
Show resolved
Hide resolved
|
I ran yarn scrub, but it only removed any untracked changes and not everything I've checked in. I've made a new branch and applied my change, but I still got a request to make all the change files. I decided to just continue with this specific branch. |
|
What's the output of |
|
origin https://github.com/OfficeDev/office-ui-fabric-react.git (fetch) |
|
Interesting, then how are you getting your changes from your local copy to your fork? (https://github.com/chang47/office-ui-fabric-react) |
|
Oops sorry, I was looking at a clone of the original Fabric repo. Here's the result of the command on my fork: |
|
You might be having issues because you're missing an After doing that, try deleting and re-generating the change files again and hopefully it will work. |
Asset size changes
Baseline commit: 899983a2838318f02d45ef2d92c03cb188b736bd (build) |
|
That did the trick! Thanks for the help. I've removed the extra change files. |
|
Great! @kenotron It would be good to add a case in beachball to handle things like this better, when someone has only one remote which is a fork not the main repo. (I think beachball concluded the change files were needed because the fork's master is out of date with OUFR master.) You could potentially detect it by looking at the repository setting in package.json. |
Component Perf AnalysisNo significant results to display. All results
|
change/office-ui-fabric-react-2019-08-28-12-35-53-v-mare-Dialog-dummy-text-change.json
Outdated
Show resolved
Hide resolved
|
This change now looks good, @ecraig12345 - can you confirm that this is okay? |
|
Hello @kenotron! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
@kenotron The code issues have been addressed, but there's still the important question of whether this is really an API we want to add (since once we add it, we're stuck with it). I sent you a link on chat to a discussion about that. |
kenotron
dismissed
their stale review
Code owner requested to approve. I'm dismissing my review.
|
@ecraig12345 I was wondering if you have reached any conclusion about this. Thanks! |
|
@ecraig12345 @kenotron is there any update on this? I would like to reach a resolution on this as it's a blocking change that I require. |
|
@ecraig12345 is currently on vacation, and she's really the code owner to ultimately make this call. Sorry, I think she'll be back next week. |
|
@ecraig12345 Hi I was wondering if you had a chance to discuss this? Thanks! |
|
🎉 Handy links: |
Pull request checklist
$ yarn changeDescription of changes
Currently the Message Bar uses predefined icons from Fabric. However users might want to customize the icons to be something else. This change adds the ability for the user to pass in a string to override the existing icons with another icon that they have created an icon mapping to
Focus areas to test
Message Bar Page. I specifically checked if adding a new Icon will override the existing ones used.
Microsoft Reviewers: Open in CodeFlow