-
Notifications
You must be signed in to change notification settings - Fork 78
Alcail/chat message contextMenu and editBox accessible through keyboard #1025
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 all commits
44caee7
8b7f4ee
d3069f6
8812a2b
701c045
17c69ea
fe75154
108c2c5
b258cc3
69a9bc6
2494009
a68febe
1cfe95f
a338222
5968121
e1aa45a
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": "added ariaLabel in InputBoxButtonProps and strings for EditBox buttons in MessageThreadStrings", | ||
| "packageName": "@internal/react-components", | ||
| "email": "alcail@microsoft.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "none", | ||
| "comment": "updated Sendbox and EditBox styles", | ||
| "packageName": "@internal/react-composites", | ||
| "email": "alcail@microsoft.com", | ||
| "dependentChangeType": "none" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "none", | ||
| "comment": "updated Sendbox custom styles", | ||
| "packageName": "@internal/storybook", | ||
| "email": "alcail@microsoft.com", | ||
| "dependentChangeType": "none" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| // Licensed under the MIT license. | ||
|
|
||
| import React, { useState, ReactNode, FormEvent, useCallback } from 'react'; | ||
| import { Stack, TextField, mergeStyles, IStyle, ITextField, concatStyleSets } from '@fluentui/react'; | ||
| import { Stack, TextField, mergeStyles, IStyle, ITextField, concatStyleSets, IconButton } from '@fluentui/react'; | ||
| import { BaseCustomStyles } from '../types'; | ||
| import { | ||
| inputBoxStyle, | ||
|
|
@@ -130,28 +130,29 @@ export type InputBoxButtonProps = { | |
| onClick: (e: React.MouseEvent<HTMLDivElement, MouseEvent>) => void; | ||
| className?: string; | ||
| id?: string; | ||
| ariaLabel?: string; | ||
|
Contributor
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. Instead,
Contributor
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. after discussion and the fact that onRenderIcon is not defined the same way between the 2 props due to rendering different icon depending on hovering state, I will not do this inheritance. |
||
| }; | ||
|
|
||
| /** | ||
| * @private | ||
| */ | ||
| export const InputBoxButton = (props: InputBoxButtonProps): JSX.Element => { | ||
| const { onRenderIcon, onClick, className, id } = props; | ||
| const [isMouseOverSendIcon, setIsMouseOverSendIcon] = useState(false); | ||
| const { onRenderIcon, onClick, ariaLabel, className, id } = props; | ||
| const [isMouseOverIcon, setIsMouseOverIcon] = useState(false); | ||
|
Contributor
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. nit. Something feels very wrong about us holding onto the state that tells us if we are hovering over the icon.
Contributor
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. Agree, but was part of component before, so I did not want to touch that. Now that we want the props to be an extension of IButtonProps, I have to revisit this
Contributor
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. the issue here is that we have a complete different icon when overring
Contributor
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. left a question to FluentUI team. Will follow up |
||
| const mergedButtonStyle = mergeStyles(inputButtonStyle, className); | ||
| return ( | ||
| <div | ||
| <IconButton | ||
| className={mergedButtonStyle} | ||
| ariaLabel={ariaLabel} | ||
| onClick={onClick} | ||
| id={id} | ||
| onMouseEnter={() => { | ||
| setIsMouseOverSendIcon(true); | ||
| setIsMouseOverIcon(true); | ||
| }} | ||
| onMouseLeave={() => { | ||
| setIsMouseOverSendIcon(false); | ||
| setIsMouseOverIcon(false); | ||
| }} | ||
| > | ||
| {onRenderIcon(props, isMouseOverSendIcon)} | ||
| </div> | ||
| onRenderIcon={() => onRenderIcon(props, isMouseOverIcon)} | ||
| /> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,14 +78,12 @@ export const textFieldStyle = (errorColor: string, hasErrorMessage: boolean, dis | |
| */ | ||
| export const inputButtonStyle = mergeStyles({ | ||
| color: 'grey', | ||
| cursor: 'pointer', | ||
|
Contributor
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. nit: I would have preferred these style changes to be in a separate PR. Most of the e2e snapshot changes are due to these, unrelated to the non-trivial code changes elsewhere in this PR.
Contributor
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. This is part of the PR. Styles need to be changed due to using IconButton |
||
| display: 'flex', | ||
| justifyContent: 'center', | ||
| margin: 'auto', | ||
| top: '0', | ||
| bottom: '0', | ||
| width: '1.0625rem', | ||
| height: '1.0625rem' | ||
| height: '1.0625rem', | ||
| '&:hover': { | ||
| backgroundColor: 'transparent' | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
|
|
@@ -94,6 +92,7 @@ export const inputButtonStyle = mergeStyles({ | |
| export const inputButtonContainerStyle = (rtl?: boolean): string => | ||
| mergeStyles({ | ||
| display: 'flex', | ||
| alignItems: 'baseline', | ||
| justifyContent: 'center', | ||
| position: 'absolute', | ||
| margin: 'auto', | ||
|
|
||
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.
Doesn't lint complain about dependencies here?
editTextFieldRef.currentshould be in the dependency array.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.
no complain, having an empty array will go through this only when component mount, and that's what we want to be able to have the cursor on the TextTield when EditBox opens