-
Notifications
You must be signed in to change notification settings - Fork 1
Core 1389 toc button accessibility issues #2615
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
base: main
Are you sure you want to change the base?
Changes from all commits
51bcc27
d9e15c1
e2977d6
191a2f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { toolbarIconStyles } from '../Toolbar/iconStyles'; | ||
| import type { InnerProps } from './types'; | ||
| import styled, { css } from 'styled-components/macro'; | ||
| import { toolbarIconColor } from '../constants'; | ||
| import { toolbarDefaultButton, toolbarDefaultText } from '../Toolbar/styled'; | ||
|
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. These could be sorted I suppose |
||
| import theme from '../../../theme'; | ||
|
|
||
| // tslint:disable-next-line:variable-name | ||
| export const ButtonText = styled.span` | ||
| ${toolbarDefaultText} | ||
| margin: 0; | ||
| padding: 0; | ||
| `; | ||
|
|
||
| // tslint:disable-next-line: variable-name | ||
| export const CloseButton = styled.button` | ||
| color: ${toolbarIconColor.base}; | ||
| border: none; | ||
| padding: 0; | ||
| background: none; | ||
| overflow: visible; | ||
| cursor: pointer; | ||
| display: block; | ||
|
|
||
| :hover { | ||
| color: ${toolbarIconColor.darker}; | ||
| } | ||
|
|
||
| ${theme.breakpoints.mobileMedium(css` | ||
| display: none; | ||
| `)} | ||
| `; | ||
|
|
||
| // tslint:disable-next-line:variable-name | ||
| export const OpenButton = styled.button<{isOpen: InnerProps['isOpen'], isActive: boolean }>` | ||
| background: none; | ||
| ${toolbarDefaultButton} | ||
| color: ${toolbarIconColor.base}; | ||
| display: flex; | ||
| border: none; | ||
| padding: 0; | ||
| overflow: visible; | ||
| cursor: pointer; | ||
|
|
||
| :hover { | ||
| color: ${toolbarIconColor.darker}; | ||
| } | ||
|
|
||
| > svg { | ||
| ${toolbarIconStyles}; | ||
| } | ||
|
|
||
|
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. There's no more hidden version of this in desktop. I also refactored two mobile functions into one. |
||
| ${(props) => props.isOpen === null && theme.breakpoints.mobile(css` | ||
| display: ${props.isActive ? 'none' : 'flex'}; | ||
| `)} | ||
|
|
||
| ${(props) => props.hideMobile && theme.breakpoints.mobileMedium(css` | ||
| display: none; | ||
| `)} | ||
| `; | ||
|
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. Everything here is moved here unchanged. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import React from 'react'; | ||
| import { connect } from 'react-redux'; | ||
| import { useIntl } from 'react-intl'; | ||
| import styled, { css } from 'styled-components/macro'; | ||
| import SearchIcon from '../../../../assets/SearchIcon'; | ||
| import theme from '../../../theme'; | ||
| import * as actions from '../../actions'; | ||
| import * as searchActions from '../../search/actions'; | ||
| import * as searchSelectors from '../../search/selectors'; | ||
| import { AppState, Dispatch } from '../../../types'; | ||
| import type { InnerProps, MiddleProps } from './types'; | ||
| import { OpenButton, ButtonText } from './Buttons'; | ||
|
|
||
| const closedSearchMessage = 'i18n:toolbar:search:toggle:closed'; | ||
| const openSearchMessage = 'i18n:toolbar:search:toggle:opened'; | ||
|
|
||
| // tslint:disable-next-line:variable-name | ||
| export const SearchControl = ({ message, children, ...props }: React.PropsWithChildren<InnerProps>) => | ||
| <OpenButton | ||
| aria-label={useIntl().formatMessage({ id: message })} | ||
| hideMobile={true} | ||
| {...props} | ||
| > | ||
| <SearchIcon /> | ||
| <ButtonText> | ||
| {useIntl().formatMessage({ id: 'i18n:toolbar:search:text' })} | ||
| </ButtonText> | ||
| {children} | ||
| </OpenButton>; | ||
|
|
||
|
|
||
| const searchConnector = connect( | ||
| (state: AppState) => ({ | ||
| hasQuery: !!searchSelectors.query(state), | ||
| isOpen: searchSelectors.searchResultsOpen(state), | ||
| }), | ||
| (dispatch: Dispatch) => ({ | ||
| close: () => dispatch(searchActions.clearSearch()), | ||
| open: () => { | ||
| dispatch(actions.closeToc()); | ||
| dispatch(searchActions.openSearchInSidebar()); | ||
| }, | ||
| }) | ||
| ); | ||
|
|
||
| // Search in sidebar experiment | ||
| // tslint:disable-next-line:variable-name | ||
| export const lockSearchControlState = (isOpen: boolean, Control: React.ComponentType<InnerProps>) => | ||
| searchConnector(styled(({ open, close, hasQuery, desktop = false, ...props }: MiddleProps & { | ||
| desktop?: boolean; | ||
| hasQuery: boolean; | ||
| }) => <Control | ||
| {...props} | ||
| data-testid={`${desktop ? 'desktop' : 'mobile'}-search-button`} | ||
| message={isOpen ? openSearchMessage : closedSearchMessage} | ||
| data-analytics-label={isOpen ? 'Click to close Search' : 'Click to open Search'} | ||
| onClick={(desktop && hasQuery) || isOpen ? close : open} | ||
| isOpen={desktop ? hasQuery || props.isOpen : props.isOpen} | ||
|
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 guess you just copied this but this logic is pretty confusing, especially with isOpen vs props.isOpen |
||
| isActive={Boolean(props.showActivatedState) && isOpen} | ||
| />)` | ||
| ${({ desktop }: { desktop: boolean }) => !desktop && theme.breakpoints.desktop(css` | ||
| display: none; | ||
| `)} | ||
| ${({ desktop }: { desktop: boolean }) => desktop && theme.breakpoints.mobile(css` | ||
| display: none; | ||
| `)} | ||
| `); | ||
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.
I made a directory and split this file into pieces