Skip to content

C
coopwire-hypothesis
Commit 15a628cd authored by Lyza Danger Gardner's avatar Lyza Danger Gardner Committed by Lyza Gardner
Browse files
Options

Update SelectionTabs component for a11y, shared components 

* Use `LabeledButton` instead of custom `<button>`
* Use `Icon`, `Frame` components where useful
* Reduce local/custom CSS
* Modernize tests
* Add title to `Icon` for a11y

Fixes https://github.com/hypothesis/support/issues/241

Part of https://github.com/hypothesis/client/issues/3876

Part of https://github.com/hypothesis/frontend-shared/issues/232
parent afca5e8f
Hide whitespace changes
Inline Side-by-side
Showing with 141 additions and 158 deletions
src/sidebar/components/SelectionTabs.js
View file @ 15a628cd
import { LabeledButton, SvgIcon } from '@hypothesis/frontend-shared';
import { Frame, Icon, LabeledButton } from '@hypothesis/frontend-shared';
import classnames from 'classnames';
import { applyTheme } from '../helpers/theme';
......@@ -11,17 +11,19 @@ import { withServices } from '../service-context';
*/
/**
* @typedef {import('preact').ComponentChildren} Children
*
* @typedef TabProps
* @prop {object} children - Child components.
* @prop {number} count - The total annotations for this tab.
* @prop {Children} children
* @prop {number} count - The total annotations for this tab
* @prop {boolean} isSelected - Is this tab currently selected?
* @prop {boolean} isWaitingToAnchor - Are there any annotations still waiting to anchor?
* @prop {string} label - A string label to use for `aria-label` and `title`
* @prop {() => any} onSelect - Callback to invoke when this tab is selected.
* @prop {string} label - A string label to use for a11y
* @prop {() => any} onSelect - Callback to invoke when this tab is selected
*/
/**
* Display name of the tab and annotation count.
* Display name of the tab and annotation count
*
* @param {TabProps} props
*/
......@@ -42,28 +44,32 @@ function Tab({
const title = count > 0 ? `${label} (${count} available)` : label;
return (
<div>
<button
className={classnames('SelectionTabs__type', {
'is-selected': isSelected,
})}
// Listen for `onMouseDown` so that the tab is selected when _pressed_
// as this makes the UI feel faster. Also listen for `onClick` as a fallback
// to enable selecting the tab via other input methods.
onClick={selectTab}
onMouseDown={selectTab}
role="tab"
tabIndex={0}
title={title}
aria-label={title}
aria-selected={isSelected.toString()}
>
<LabeledButton
classes={classnames('u-color-text', 'SelectionTab', {
'is-selected': isSelected,
})}
// Listen for `onMouseDown` so that the tab is selected when _pressed_
// as this makes the UI feel faster. Also listen for `onClick` as a fallback
// to enable selecting the tab via other input methods.
onClick={selectTab}
onMouseDown={selectTab}
pressed={!!isSelected}
role="tab"
tabIndex={0}
title={title}
>
<>
{children}
{count > 0 && !isWaitingToAnchor && (
<span className="SelectionTabs__count"> {count}</span>
<span
className="u-font--xsmall"
style="position:relative;bottom:3px;left:2px"
>
{count}
</span>
)}
</button>
</div>
</>
</LabeledButton>
);
}
......@@ -75,7 +81,7 @@ function Tab({
*/
/**
* Tabbed display of annotations and notes.
* Tabbed display of annotations and notes
*
* @param {SelectionTabsProps} props
*/
......@@ -103,8 +109,11 @@ function SelectionTabs({ annotationsService, isLoading, settings }) {
const showNotesUnavailableMessage = selectedTab === 'note' && noteCount === 0;
return (
<div className="SelectionTabs-container">
<div className="SelectionTabs" role="tablist">
<div className="hyp-u-vertical-spacing--4 SelectionTabs__container">
<div
className="hyp-u-layout-row hyp-u-horizontal-spacing--6 SelectionTabs"
role="tablist"
>
<Tab
count={annotationCount}
isWaitingToAnchor={isWaitingToAnchorAnnotations}
......@@ -138,6 +147,7 @@ function SelectionTabs({ annotationsService, isLoading, settings }) {
{selectedTab === 'note' && settings.enableExperimentalNewNoteButton && (
<div className="hyp-u-layout-row--justify-right">
<LabeledButton
data-testid="new-note-button"
icon="add"
onClick={() => annotationsService.createPageNote()}
variant="primary"
......@@ -148,22 +158,26 @@ function SelectionTabs({ annotationsService, isLoading, settings }) {
</div>
)}
{!isLoading && showNotesUnavailableMessage && (
<div className="SelectionTabs__message">
There are no page notes in this group.
</div>
<Frame classes="u-text--centered">
<span data-testid="notes-unavailable-message">
There are no page notes in this group.
</span>
</Frame>
)}
{!isLoading && showAnnotationsUnavailableMessage && (
<div className="SelectionTabs__message">
There are no annotations in this group.
<br />
Create one by selecting some text and clicking the{' '}
<SvgIcon
name="annotate"
inline={true}
className="SelectionTabs__icon"
/>{' '}
button.
</div>
<Frame classes="u-text--centered">
<span data-testid="annotations-unavailable-message">
There are no annotations in this group.
<br />
Create one by selecting some text and clicking the{' '}
<Icon
classes="hyp-u-margin--1 u-inline"
name="annotate"
title="Annotate"
/>{' '}
button.
</span>
</Frame>
)}
</div>
);
......
src/sidebar/components/test/SelectionTabs-test.js
View file @ 15a628cd
......@@ -55,35 +55,35 @@ describe('SelectionTabs', () => {
$imports.$restore();
});
const unavailableMessage = wrapper =>
wrapper.find('.SelectionTabs__message').text();
it('should display the tabs and counts of annotations and notes', () => {
const wrapper = createComponent();
const tabs = wrapper.find('button');
const annotationTab = wrapper.find('Tab[label="Annotations"]');
const noteTab = wrapper.find('Tab[label="Page notes"]');
assert.include(tabs.at(0).text(), 'Annotations');
assert.equal(tabs.at(0).find('.SelectionTabs__count').text(), 123);
assert.include(annotationTab.text(), 'Annotations');
assert.include(annotationTab.text(), '123');
assert.include(tabs.at(1).text(), 'Page Notes');
assert.equal(tabs.at(1).find('.SelectionTabs__count').text(), 456);
assert.include(noteTab.text(), 'Page Notes');
assert.include(noteTab.text(), '456');
});
describe('Annotations tab', () => {
it('should display annotations tab as selected when it is active', () => {
const wrapper = createComponent();
const annotationTab = wrapper.find('Tab[label="Annotations"]');
const noteTab = wrapper.find('Tab[label="Page notes"]');
const tabs = wrapper.find('button');
assert.isTrue(tabs.at(0).hasClass('is-selected'));
assert.equal(tabs.at(0).prop('aria-selected'), 'true');
assert.equal(tabs.at(1).prop('aria-selected'), 'false');
assert.isTrue(annotationTab.find('LabeledButton').props().pressed);
assert.isFalse(noteTab.find('LabeledButton').props().pressed);
});
it('should not display the add-page-note button when the annotations tab is active', () => {
fakeSettings.enableExperimentalNewNoteButton = true;
const wrapper = createComponent();
assert.equal(wrapper.find('LabeledButton').length, 0);
assert.equal(
wrapper.find('LabeledButton[data-testid="new-note-button"]').length,
0
);
});
});
......@@ -92,11 +92,16 @@ describe('SelectionTabs', () => {
fakeStore.selectedTab.returns('note');
const wrapper = createComponent();
const tabs = wrapper.find('button');
const annotationTabButton = wrapper
.find('Tab[label="Annotations"]')
.find('LabeledButton');
const noteTabButton = wrapper
.find('Tab[label="Page notes"]')
.find('LabeledButton');
assert.isTrue(tabs.at(1).hasClass('is-selected'));
assert.equal(tabs.at(1).prop('aria-selected'), 'true');
assert.equal(tabs.at(0).prop('aria-selected'), 'false');
assert.isTrue(noteTabButton.find('button').hasClass('is-selected'));
assert.isTrue(noteTabButton.prop('pressed'));
assert.isFalse(annotationTabButton.prop('pressed'));
});
describe('Add Page Note button', () => {
......@@ -106,7 +111,9 @@ describe('SelectionTabs', () => {
const wrapper = createComponent();
assert.isFalse(wrapper.find('LabeledButton').exists());
assert.isFalse(
wrapper.find('LabeledButton[data-testid="new-note-button"]').exists()
);
});
it('should display the add-page-note button when the associated setting is enabled', () => {
......@@ -115,7 +122,9 @@ describe('SelectionTabs', () => {
const wrapper = createComponent();
assert.isTrue(wrapper.find('LabeledButton').exists());
assert.isTrue(
wrapper.find('LabeledButton[data-testid="new-note-button"]').exists()
);
});
it('should apply background-color styling from settings', () => {
......@@ -129,7 +138,9 @@ describe('SelectionTabs', () => {
const wrapper = createComponent();
const button = wrapper.find('LabeledButton');
const button = wrapper.find(
'LabeledButton[data-testid="new-note-button"]'
);
assert.deepEqual(button.prop('style'), { backgroundColor: '#00f' });
});
......@@ -138,7 +149,10 @@ describe('SelectionTabs', () => {
fakeStore.selectedTab.returns('note');
const wrapper = createComponent();
wrapper.find('LabeledButton').props().onClick();
wrapper
.find('LabeledButton[data-testid="new-note-button"]')
.props()
.onClick();
assert.calledOnce(fakeAnnotationsService.createPageNote);
});
......@@ -151,8 +165,8 @@ describe('SelectionTabs', () => {
const wrapper = createComponent();
const tabs = wrapper.find('button');
assert.equal(tabs.length, 3);
const orphanTab = wrapper.find('Tab[label="Orphans"]');
assert.isTrue(orphanTab.exists());
});
it('should display orphans tab as selected when it is active', () => {
......@@ -161,11 +175,8 @@ describe('SelectionTabs', () => {
const wrapper = createComponent();
const tabs = wrapper.find('button');
assert.isTrue(tabs.at(2).hasClass('is-selected'));
assert.equal(tabs.at(2).prop('aria-selected'), 'true');
assert.equal(tabs.at(1).prop('aria-selected'), 'false');
assert.equal(tabs.at(0).prop('aria-selected'), 'false');
const orphanTab = wrapper.find('Tab[label="Orphans"]');
assert.isTrue(orphanTab.find('LabeledButton').prop('pressed'));
});
it('should not display orphans tab if there are 0 orphans', () => {
......@@ -173,38 +184,21 @@ describe('SelectionTabs', () => {
const wrapper = createComponent();
const tabs = wrapper.find('button');
assert.equal(tabs.length, 2);
const orphanTab = wrapper.find('Tab[label="Orphans"]');
assert.isFalse(orphanTab.exists());
});
});
describe('tab display and counts', () => {
it('should render `title` and `aria-label` attributes for tab buttons, with counts', () => {
fakeStore.orphanCount.returns(1);
const wrapper = createComponent();
const tabs = wrapper.find('button');
assert.equal(
tabs.at(0).prop('aria-label'),
'Annotations (123 available)'
);
assert.equal(tabs.at(0).prop('title'), 'Annotations (123 available)');
assert.equal(tabs.at(1).prop('aria-label'), 'Page notes (456 available)');
assert.equal(tabs.at(1).prop('title'), 'Page notes (456 available)');
assert.equal(tabs.at(2).prop('aria-label'), 'Orphans (1 available)');
assert.equal(tabs.at(2).prop('title'), 'Orphans (1 available)');
});
it('should not render count in `title` and `aria-label` for page notes tab if there are no page notes', () => {
it('should not render count if there are no page notes', () => {
fakeStore.noteCount.returns(0);
const wrapper = createComponent({});
const tabs = wrapper.find('button');
const noteTab = wrapper.find('Tab[label="Page notes"]');
assert.equal(tabs.at(1).prop('aria-label'), 'Page notes');
assert.equal(tabs.at(1).prop('title'), 'Page notes');
assert.equal(noteTab.text(), 'Page Notes');
});
it('should not display a message when its loading annotation count is 0', () => {
......@@ -212,7 +206,9 @@ describe('SelectionTabs', () => {
const wrapper = createComponent({
isLoading: true,
});
assert.isFalse(wrapper.exists('.annotation-unavailable-message__label'));
assert.isFalse(
wrapper.exists('[data-testid="annotations-unavailable-message"]')
);
});
it('should not display a message when its loading notes count is 0', () => {
......@@ -221,7 +217,9 @@ describe('SelectionTabs', () => {
const wrapper = createComponent({
isLoading: true,
});
assert.isFalse(wrapper.exists('.SelectionTabs__message'));
assert.isFalse(
wrapper.exists('[data-testid="notes-unavailable-message"]')
);
});
it('should not display the longer version of the no annotations message when there are no annotations and isWaitingToAnchorAnnotations is true', () => {
......@@ -230,16 +228,19 @@ describe('SelectionTabs', () => {
const wrapper = createComponent({
isLoading: false,
});
assert.isFalse(wrapper.exists('.SelectionTabs__message'));
assert.isFalse(
wrapper.exists('[data-testid="annotations-unavailable-message"]')
);
});
it('should display the longer version of the no notes message when there are no notes', () => {
fakeStore.selectedTab.returns('note');
fakeStore.noteCount.returns(0);
const wrapper = createComponent({});
assert.include(
unavailableMessage(wrapper),
'There are no page notes in this group.'
wrapper.find('[data-testid="notes-unavailable-message"]').text(),
'There are no page notes in this group'
);
});
......@@ -247,12 +248,8 @@ describe('SelectionTabs', () => {
fakeStore.annotationCount.returns(0);
const wrapper = createComponent({});
assert.include(
unavailableMessage(wrapper),
'There are no annotations in this group.'
);
assert.include(
unavailableMessage(wrapper),
'Create one by selecting some text and clicking the'
wrapper.find('[data-testid="annotations-unavailable-message"]').text(),
'There are no annotations in this group'
);
});
});
......
src/styles/sidebar/components/SelectionTabs.scss
View file @ 15a628cd
@use '@hypothesis/frontend-shared/styles/components/buttons';
@use '../../mixins/layout';
@use '../../mixins/utils';
@use '../../variables' as var;
.SelectionTabs-container {
@include layout.vertical-rhythm;
.SelectionTabs__container {
// FIXME: This should be a margin, and it should be handled by the parent,
// but needs to be considered carefully because applying vertical rhythm to
// this component's parent messes with the calculations in the virtualized
// thread list. Needs another pass. Note also that it is `10px` (and looks
// unbalanced at the standard vertical rhythm size of `1em`)
// unbalanced at the standard vertical rhythm size of `1rem`)
padding-bottom: 10px;
}
.SelectionTabs {
@include layout.row;
@include layout.horizontal-rhythm(20px);
}
.SelectionTabs__icon {
color: var.$grey-mid;
margin: 0 var.$layout-space--xxsmall;
}
.SelectionTabs__type {
@include buttons.reset;
color: var.$color-text;
cursor: pointer;
min-width: 85px;
min-height: 18px;
// Give the tab a radius to allow :focus styling to appear similar to that of buttons
border-radius: var.$border-radius;
.SelectionTab {
margin: 0;
padding: 0;
font-weight: normal;
background-color: transparent;
user-select: none;
min-width: 5.25rem;
&:hover {
&:hover:not([disabled]) {
color: var.$color-link-hover;
}
}
.SelectionTabs__type.is-selected {
font-weight: bold;
}
.SelectionTabs__count {
@include utils.font--xsmall;
position: relative;
bottom: 3px;
}
.SelectionTabs__empty-message {
position: relative;
top: 10px;
}
.SelectionTabs__type--orphan {
margin-left: -5px;
}
.SelectionTabs__message {
@include utils.border;
color: var.$color-text;
padding: 2em;
text-align: center;
&.is-selected {
font-weight: bold;
}
}
/** Clean theme affordances */
......
src/styles/util.scss
View file @ 15a628cd
......@@ -4,6 +4,9 @@
// Utility classes
// These will be extracted and considered when developing typography patterns
.u-font--xsmall {
@include utils.font--xsmall;
}
.u-font--small {
@include utils.font--small;
......@@ -66,6 +69,16 @@
color: var.$color-text;
}
// Layout
.u-inline {
display: inline;
}
.u-text--centered {
text-align: center;
}
// TODO: This is a temporary utility class to allow elements in the sidebar
// (e.g. panels, thread cards, etc.)
// to apply margins such that they are evenly spaced. In the future, the
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment