-
Notifications
You must be signed in to change notification settings - Fork 78
Fix control bar button label styling. Fix screenshare button hover and checked styling. #1178
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
Changes from 11 commits
e9d2f7d
ed66c1c
e29af7e
ba8dcf8
d0b3ba3
f792107
e64d9b8
2fd3885
ffa895b
9d7bd77
ce15974
1b8e313
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "prerelease", | ||
| "comment": "Update screenshare button background when active", | ||
| "packageName": "@internal/react-components", | ||
| "email": "2684369+JamesBurnside@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "none", | ||
| "comment": "Moved code", | ||
| "packageName": "@internal/react-composites", | ||
| "email": "2684369+JamesBurnside@users.noreply.github.com", | ||
| "dependentChangeType": "none" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "none", | ||
| "comment": "update jest snapshots", | ||
| "packageName": "@internal/storybook", | ||
| "email": "2684369+JamesBurnside@users.noreply.github.com", | ||
| "dependentChangeType": "none" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,9 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| import React, { useCallback } from 'react'; | ||
| import { | ||
| DefaultButton, | ||
| IButtonProps, | ||
| IRenderFunction, | ||
| Text, | ||
| concatStyleSets, | ||
| mergeStyles, | ||
| IButtonStyles | ||
| } from '@fluentui/react'; | ||
| import { useTheme } from '../theming'; | ||
| import { controlButtonLabelStyles, controlButtonStyles } from './styles/ControlBar.styles'; | ||
| import React from 'react'; | ||
| import { DefaultButton, IButtonProps, IRenderFunction, concatStyleSets, IButtonStyles } from '@fluentui/react'; | ||
| import { controlButtonStyles } from './styles/ControlBar.styles'; | ||
| import { ControlButtonTooltip } from './ControlButtonTooltip'; | ||
|
|
||
| /** | ||
|
|
@@ -123,22 +114,10 @@ const DefaultRenderIcon = (props?: ControlBarButtonProps): JSX.Element | null => | |
| */ | ||
| export const ControlBarButton = (props: ControlBarButtonProps): JSX.Element => { | ||
| const componentStyles = concatStyleSets(controlButtonStyles, props.styles ?? {}); | ||
| const theme = useTheme(); | ||
|
|
||
| const labelText = | ||
| props?.text ?? props?.strings?.label ?? (props?.checked ? props?.strings?.onLabel : props?.strings?.offLabel); | ||
|
|
||
| const DefaultRenderText = useCallback(() => { | ||
| return ( | ||
| <Text | ||
| key={props?.labelKey} | ||
| className={mergeStyles(controlButtonLabelStyles, theme.palette.neutralPrimary, props?.styles?.label)} | ||
| > | ||
| {labelText} | ||
| </Text> | ||
| ); | ||
| }, [labelText, props?.labelKey, props?.styles?.label, theme]); | ||
|
|
||
| const tooltipContent = | ||
| props?.strings?.tooltipContent ?? | ||
| (props?.disabled | ||
|
|
@@ -154,9 +133,11 @@ export const ControlBarButton = (props: ControlBarButtonProps): JSX.Element => { | |
| <DefaultButton | ||
| {...props} | ||
| styles={componentStyles} | ||
| onRenderText={props.showLabel ? props.onRenderText ?? DefaultRenderText : undefined} | ||
| onRenderText={props.showLabel && props.onRenderText ? props.onRenderText : undefined} | ||
| onRenderIcon={props.onRenderIcon ?? DefaultRenderIcon} | ||
| /> | ||
| > | ||
| {props.showLabel ? labelText : <></>} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NOTE: No longer using a custom render function (so get the styling from props correctly) |
||
| </DefaultButton> | ||
| </ControlButtonTooltip> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| import React from 'react'; | ||
| import { useLocale } from '../localization'; | ||
| import { ControlBarButton, ControlBarButtonProps } from './ControlBarButton'; | ||
| import { Icon } from '@fluentui/react'; | ||
| import { DefaultPalette, IButtonStyles, Icon, Theme, useTheme } from '@fluentui/react'; | ||
|
|
||
| /** | ||
| * Strings of {@link ScreenShareButton} that can be overridden. | ||
|
|
@@ -56,9 +56,13 @@ export const ScreenShareButton = (props: ScreenShareButtonProps): JSX.Element => | |
| const localeStrings = useLocale().strings.screenShareButton; | ||
| const strings = { ...localeStrings, ...props.strings }; | ||
|
|
||
| const theme = useTheme(); | ||
| const styles = screenshareButtonStyles(theme); | ||
|
|
||
| return ( | ||
| <ControlBarButton | ||
| {...props} | ||
| styles={styles} | ||
| onClick={props.onToggleScreenShare ?? props.onClick} | ||
| onRenderOnIcon={props.onRenderOnIcon ?? onRenderScreenShareOnIcon} | ||
| onRenderOffIcon={props.onRenderOffIcon ?? onRenderScreenShareOffIcon} | ||
|
|
@@ -67,3 +71,18 @@ export const ScreenShareButton = (props: ScreenShareButtonProps): JSX.Element => | |
| /> | ||
| ); | ||
| }; | ||
|
|
||
| const screenshareButtonStyles = (theme: Theme): IButtonStyles => ({ | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NOTE: Now in components and not composites so our customers get this behavior |
||
| rootChecked: { | ||
| background: theme.palette.themePrimary, | ||
| color: DefaultPalette.white, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious about the usage of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left this as is (this code was just moved from one file to another) but also curious on this too if anyone knows why default palette is used
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think @PorterNan worked on this originally. |
||
| ':focus::after': { outlineColor: `${DefaultPalette.white} !important` } // added !important to avoid override by FluentUI button styles | ||
| }, | ||
| rootCheckedHovered: { | ||
| background: theme.palette.themePrimary, | ||
| color: DefaultPalette.white, | ||
| ':focus::after': { outlineColor: `${DefaultPalette.white} !important` } // added !important to avoid override by FluentUI button styles | ||
| }, | ||
| labelHovered: { color: DefaultPalette.white }, | ||
| labelChecked: { color: DefaultPalette.white } | ||
| }); | ||
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.
Can we be specific in this comment about which code was removed?
Helps when we go over the combined changelog later.
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.
This is a no-op for this package (nothing changes from the perspective of the composites package) so type is 'none'. When the changelog is generated it omits these
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.
Oh cool. Didn't know that.