Skip to content
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

Use LinkButton in Excerpt #3268

Merged
merged 1 commit into from Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/sidebar/components/Excerpt.js
Expand Up @@ -5,6 +5,8 @@ import observeElementSize from '../util/observe-element-size';
import { withServices } from '../service-context';
import { applyTheme } from '../helpers/theme';

import { LinkButton } from '../../shared/components/buttons';

/**
* @typedef InlineControlsProps
* @prop {boolean} isCollapsed
Expand All @@ -23,17 +25,18 @@ function InlineControls({ isCollapsed, setCollapsed, linkStyle = {} }) {

return (
<div className="Excerpt__inline-controls">
<span className="Excerpt__toggle-link">
<button
className="Excerpt__toggle-button"
<div className="Excerpt__toggle-container">
<LinkButton
className="InlineLinkButton"
onClick={() => setCollapsed(!isCollapsed)}
aria-expanded={!isCollapsed}
aria-label="Toggle visibility of full excerpt text"
expanded={!isCollapsed}
title="Toggle visibility of full excerpt text"
style={linkStyle}
variant="dark"
>
{toggleLabel}
</button>
</span>
</LinkButton>
</div>
</div>
);
}
Expand Down
30 changes: 21 additions & 9 deletions src/sidebar/components/test/Excerpt-test.js
Expand Up @@ -115,36 +115,48 @@ describe('Excerpt', () => {
});

context('when inline controls are enabled', () => {
const getToggleButton = wrapper =>
wrapper.find(
'LinkButton[title="Toggle visibility of full excerpt text"]'
);

it('displays inline controls if collapsed', () => {
const wrapper = createExcerpt({ inlineControls: true }, TALL_DIV);
assert.isTrue(wrapper.exists('.Excerpt__inline-controls'));
assert.isTrue(wrapper.exists('InlineControls'));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to not use a classname selector while I'm in here...

});

it('does not display inline controls if not collapsed', () => {
const wrapper = createExcerpt({ inlineControls: true }, SHORT_DIV);
assert.isFalse(wrapper.exists('.Excerpt__inline-controls'));
assert.isFalse(wrapper.exists('InlineControls'));
});

it('toggles the expanded state when clicked', () => {
const wrapper = createExcerpt({ inlineControls: true }, TALL_DIV);
const button = wrapper.find('.Excerpt__toggle-button');
const button = getToggleButton(wrapper);
assert.equal(getExcerptHeight(wrapper), 40);
button.simulate('click');
act(() => {
button.props().onClick();
});
wrapper.update();
assert.equal(getExcerptHeight(wrapper), 200);
});

it("sets button's default state to un-expanded", () => {
const wrapper = createExcerpt({ inlineControls: true }, TALL_DIV);
const button = wrapper.find('.Excerpt__toggle-button');
assert.equal(button.prop('aria-expanded'), false);
const button = getToggleButton(wrapper);
assert.equal(button.prop('expanded'), false);
assert.equal(button.text(), 'More');
});

it("changes button's state to expanded when clicked", () => {
const wrapper = createExcerpt({ inlineControls: true }, TALL_DIV);
wrapper.find('.Excerpt__toggle-button').simulate('click');
const button = wrapper.find('.Excerpt__toggle-button');
assert.equal(button.prop('aria-expanded'), true);
let button = getToggleButton(wrapper);
act(() => {
button.props().onClick();
});
wrapper.update();
button = getToggleButton(wrapper);
assert.equal(button.prop('expanded'), true);
assert.equal(button.text(), 'Less');
});
});
Expand Down
12 changes: 3 additions & 9 deletions src/styles/sidebar/components/Excerpt.scss
@@ -1,4 +1,3 @@
@use "../../mixins/buttons";
@use "../../variables" as var;

// the distance by which the shadow indicating a collapsed
Expand Down Expand Up @@ -34,7 +33,9 @@ $expand-duration: 0.15s;
bottom: 0;
}

.Excerpt__toggle-link {
// A container for a button with a gradient background; this gives partial
// opacity behind the More/Less button so that it is easier to read
.Excerpt__toggle-container {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this classname for clarity

padding-left: var.$layout-space;
// See https://stackoverflow.com/a/56548711/9788954 regarding
// rgba(255, 255, 255, 0) used here instead of `transparent` (for Safari)
Expand All @@ -45,13 +46,6 @@ $expand-duration: 0.15s;
);
}

// TODO the tap target here is quite small
.Excerpt__toggle-button {
@include buttons.reset-native-btn-styles;
background: transparent;
font-style: italic;
}

// a shadow displayed at the bottom of an <excerpt>s with inline controls
// disabled, which provides a hint that the excerpt is collapsed
.Excerpt__shadow {
Expand Down