-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix(react-message-bar): force links to be underlined #32850
base: master
Are you sure you want to change the base?
Conversation
📊 Bundle size reportUnchanged fixtures
|
600c392
to
5caa323
Compare
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 650 | 612 | 5000 | |
Button | mount | 317 | 300 | 5000 | |
Field | mount | 1137 | 1142 | 5000 | |
FluentProvider | mount | 707 | 751 | 5000 | |
FluentProviderWithTheme | mount | 81 | 78 | 10 | |
FluentProviderWithTheme | virtual-rerender | 41 | 41 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 77 | 84 | 10 | |
MakeStyles | mount | 881 | 857 | 50000 | |
Persona | mount | 1808 | 1759 | 5000 | |
SpinButton | mount | 1387 | 1412 | 5000 | |
SwatchPicker | mount | 1700 | 1649 | 5000 |
@@ -0,0 +1,7 @@ | |||
{ |
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.
🕵🏾♀️ visual regressions to review in the fluentuiv9 Visual Regression Report
Avatar Converged 2 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
Avatar Converged.badgeMask.chromium.png | 11 | Changed |
Avatar Converged.Badge Mask RTL.chromium.png | 5 | Changed |
Drawer 1 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
Drawer.overlay drawer full.chromium.png | 1162 | Changed |
MessageBar 9 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
MessageBar.Multiline Without Actions.chromium.png | 67 | Changed |
MessageBar.Intents.chromium.png | 67 | Changed |
MessageBar.Multiline No Actions.chromium.png | 67 | Changed |
MessageBar.Auto.chromium.png | 17 | Changed |
MessageBar.Multiline.chromium.png | 67 | Changed |
MessageBar.Intents RTL.chromium.png | 67 | Changed |
MessageBar.Intents High Contrast.chromium.png | 91 | Changed |
MessageBar.Square.chromium.png | 17 | Changed |
MessageBar.Intents Dark Mode.chromium.png | 46 | Changed |
...ts/react-message-bar/library/src/components/MessageBarBody/useMessageBarBodyStyles.styles.ts
Outdated
Show resolved
Hide resolved
This might be unrelated to this PR. But I am wondering if all links should be underlined in the context of the Design System. If that's the case, maybe it would be worth exploring if we can make this change to the |
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.
These kinds of selectors are costy and also fragile - they depend on the styles of the component. Let's introduce a context for this if it only applies to MessageBar. Similar to https://github.com/microsoft/fluentui/blob/master/packages/react-components/react-avatar/library/src/contexts/AvatarContext.ts
59c927e
to
7b584c1
Compare
}; | ||
|
||
export const MessageBarBodyContextProvider = messageBarContext.Provider; | ||
export const useMessageBarBodyContext = () => React.useContext(messageBarContext) ?? messageBarBodyContextDefaultValue; |
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.
It should be inverted i.e. react-link
has a context provider as in future another component can also tweak Link
's behavior
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.
right, makes sense, updated 👍🏻
7b584c1
to
090f9c2
Compare
According to design spec links inside MessageBar should be underlined
Previous Behavior
New Behavior
Related Issue(s)