PCF-391 The keyboard handling for the revision dropdown should be done differently#926
PCF-391 The keyboard handling for the revision dropdown should be done differently#926esanuandra wants to merge 3 commits intomozilla:mainfrom
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
90de22d to
d5beba4
Compare
d5beba4 to
d19522f
Compare
fdde6ef to
54ddcf0
Compare
b6922d9 to
031a978
Compare
13e39ea to
7c2d3b9
Compare
|
I need some help with the Snackbar tests, fetchRevisionByID and fetchRevisionsBySearch. |
7c2d3b9 to
208f150
Compare
| const activeColor = | ||
| theme === 'light' ? Colors.SecondaryActive : Colors.SecondaryActiveDark; | ||
| const captionStyle = | ||
| theme === 'light' ? captionStylesLight : captionStylesDark; |
There was a problem hiding this comment.
I'd much rather if we stick to using the light/dark mode selection tooling that Julien setup here: https://github.com/mozilla/perfcompare/blob/main/src/theme/protocolTheme.ts
Can you change your code to move the color setup for light/dark to there? That should clean up your code a good bit too.
There was a problem hiding this comment.
Oh wait, it looks like these were just copied from here:
perfcompare/src/components/Search/SearchResultsList.tsx
Lines 35 to 43 in d6ed322
Still, I think it would be good to make the light/dark mode changes now since we're already modifying this.
There was a problem hiding this comment.
@gmierz This isn't a simple ask, and beyond the scope of this PR. It isn't simple as adding
sx={{ background: 'background.default', }} to the Box and Autocomplete components
In order to implement it the way above and delete the old way, Andra would have to redo the caption styles in the styles folder because they're based on the old dark/light styling system.
I will create a meta ticket and assign it to myself to update all the dark/light styles to Julien's new, simpler system, but for now I highly recommend leaving as is to get the proper keyboard handing checked in.
There was a problem hiding this comment.
I agree with the caption style one, but the other three seem very easy to change. I checked it out locally, and everything seems to be working fine from my testing with these changes:
+ import { useTheme } from '@mui/material/styles';
import { style } from 'typestyle';
import AutocompleteInput from './AutocompleteInput';
@@ -47,6 +48,7 @@ export default function SearchInputAndResults({
listItemComponent,
}: Props) {
const mode = useAppSelector((state) => state.theme.mode);
+ const theme = useTheme();
const [recentRevisions, setRecentRevisions] = useState(
null as null | Changeset[],
@@ -76,39 +78,33 @@ export default function SearchInputAndResults({
zIndex: 100,
};
- const getListStyles = (theme: string) => {
- const backgroundColor =
- theme === 'light' ? Colors.Background300 : Colors.Background300Dark;
- const hoverColor =
- theme === 'light' ? Colors.SecondaryHover : Colors.SecondaryHoverDark;
- const activeColor =
- theme === 'light' ? Colors.SecondaryActive : Colors.SecondaryActiveDark;
+ const getListStyles = (themeMode: string) => {
const captionStyle =
- theme === 'light' ? captionStylesLight : captionStylesDark;
+ themeMode === 'light' ? captionStylesLight : captionStylesDark;
return style({
- backgroundColor,
+ backgroundColor: theme.palette.searchDropdown.background,
position: 'relative',
...sharedSelectStyles,
$nest: {
// Autocomplete option highlighting
'li[aria-selected="true"]': {
- backgroundColor: `${hoverColor} !important`,
+ backgroundColor: `${theme.palette.searchDropdown.hover} !important`,
borderRadius: '4px',
paddingTop: `${Spacing.xSmall}px`,
},
'li:hover': {
- backgroundColor: `${hoverColor} !important`,
+ backgroundColor: `${theme.palette.searchDropdown.hover} !important`,
borderRadius: '4px',
paddingTop: `${Spacing.xSmall}px`,
},
'li[data-focus="true"]': {
- backgroundColor: `${hoverColor} !important`,
+ backgroundColor: `${theme.palette.searchDropdown.hover} !important`,
borderRadius: '4px',
paddingTop: `${Spacing.xSmall}px`,
},
'li.Mui-focused': {
- backgroundColor: `${hoverColor} !important`,
+ backgroundColor: `${theme.palette.searchDropdown.hover} !important`,
borderRadius: '4px',
paddingTop: `${Spacing.xSmall}px`,
},
@@ -121,17 +117,17 @@ export default function SearchInputAndResults({
padding: `${Spacing.xSmall}px ${Spacing.Small}px`,
$nest: {
'&:hover': {
- backgroundColor: hoverColor,
+ backgroundColor: theme.palette.searchDropdown.hover,
borderRadius: '4px',
},
'&:active': {
- backgroundColor: activeColor,
+ backgroundColor: theme.palette.searchDropdown.active,
borderRadius: '4px',
},
},
},
'.item-selected': {
- backgroundColor: hoverColor,
+ backgroundColor: theme.palette.searchDropdown.hover,
borderRadius: '4px',
},
'.revision-hash': {
diff --git a/src/theme/protocolTheme.ts b/src/theme/protocolTheme.ts
index 0befd83..e418b1f 100644
--- a/src/theme/protocolTheme.ts
+++ b/src/theme/protocolTheme.ts
@@ -41,6 +41,11 @@ const lightMode = {
expandedRow: {
background: Colors.SecondaryDefault,
},
+ searchDropdown: {
+ background: Colors.Background300,
+ hover: Colors.SecondaryHover,
+ active: Colors.SecondaryActive,
+ },
};
const darkMode = {
@@ -77,6 +82,11 @@ const darkMode = {
expandedRow: {
background: Colors.Background100Dark,
},
+ searchDropdown: {
+ background: Colors.Background300Dark,
+ hover: Colors.SecondaryHoverDark,
+ active: Colors.SecondaryActiveDark,
+ },
};
| import { style } from 'typestyle'; | ||
|
|
||
| import SearchInput from './SearchInput'; | ||
| import SearchResultsList from './SearchResultsList'; |
There was a problem hiding this comment.
Is the SearchResultsList still being used somewhere after these changes? I'm not able to find any other instances of it being used apart from this file. There's also SearchResultsListItem which I can't find any usages of after these changes.
We should remove them if they're unused now.
| const activeColor = | ||
| theme === 'light' ? Colors.SecondaryActive : Colors.SecondaryActiveDark; | ||
| const captionStyle = | ||
| theme === 'light' ? captionStylesLight : captionStylesDark; |
There was a problem hiding this comment.
Oh wait, it looks like these were just copied from here:
perfcompare/src/components/Search/SearchResultsList.tsx
Lines 35 to 43 in d6ed322
Still, I think it would be good to make the light/dark mode changes now since we're already modifying this.
| }), | ||
| }; | ||
|
|
||
| function AutocompleteOption({ |
There was a problem hiding this comment.
Do we need to call these new components Autocomplete* for any reason? If possible, I'd prefer it if we give them more descriptive names related to what they'll be used for, e.g. SearchRevisionOption.
There was a problem hiding this comment.
Or maybe something like RevisionAutocompleteOption? I'm not 100% sure about what name to use here, but I think there are better options since this component is not generic, and is specific to revision options.
| compact: boolean; | ||
| } | ||
|
|
||
| function AutocompleteInput({ |
There was a problem hiding this comment.
The same is true here. RevisionInput or RevisionAutocompleteInput would be along the lines of something better.
| getOptionLabel={(options) => options.revision} | ||
| isOptionEqualToValue={(option, value) => option.id === value.id} | ||
| multiple={listItemComponent === 'checkbox'} | ||
| disableCloseOnSelect={listItemComponent === 'checkbox'} |
There was a problem hiding this comment.
I don't think this is working properly (I may be pointing to the wrong code that needs a fix). When I select something for the "new" revision when I'm using the keyboard by pressing enter, it closes the revision search dropdown instead of allowing me to continue making selections.
There was a problem hiding this comment.
Andra I suggest trying this:
multiple={listItemComponent !== 'radio'} disableCloseOnSelect={listItemComponent !== 'radio'}
| } | ||
| } | ||
| }} | ||
| renderInput={renderInput} |
There was a problem hiding this comment.
I'm not sure where this issue is coming from yet, but when I hit enter on a revision selection for the new one, the revision input field gets populated with the string of the revision that was selected. This is a bit annoying in combination with the issue above because if I want to select multiple revisions, I have to delete the revision input that was added, then I'm able to continue searching for more revisions to add.
kala-moz
left a comment
There was a problem hiding this comment.
Added my comments and suggestions to the PR.
| const activeColor = | ||
| theme === 'light' ? Colors.SecondaryActive : Colors.SecondaryActiveDark; | ||
| const captionStyle = | ||
| theme === 'light' ? captionStylesLight : captionStylesDark; |
There was a problem hiding this comment.
@gmierz This isn't a simple ask, and beyond the scope of this PR. It isn't simple as adding
sx={{ background: 'background.default', }} to the Box and Autocomplete components
In order to implement it the way above and delete the old way, Andra would have to redo the caption styles in the styles folder because they're based on the old dark/light styling system.
I will create a meta ticket and assign it to myself to update all the dark/light styles to Julien's new, simpler system, but for now I highly recommend leaving as is to get the proper keyboard handing checked in.
| getOptionLabel={(options) => options.revision} | ||
| isOptionEqualToValue={(option, value) => option.id === value.id} | ||
| multiple={listItemComponent === 'checkbox'} | ||
| disableCloseOnSelect={listItemComponent === 'checkbox'} |
There was a problem hiding this comment.
Andra I suggest trying this:
multiple={listItemComponent !== 'radio'} disableCloseOnSelect={listItemComponent !== 'radio'}
208f150 to
0458490
Compare
7785cde to
c80baad
Compare
Jira ticket - The keyboard handling for the revision dropdown should be done differently
This PR should make the navigation using the keyboard possible for searching revisions in Search component. Part of the implementation was made with the help of Cursor.