Skip to content

C
coopwire-hypothesis
Commit 96d9ff5e authored by Lyza Danger Gardner's avatar Lyza Danger Gardner
Browse files
Options

Adjust structure and vertical rhythm of annotations and threads

parent c5642ed2
Hide whitespace changes
Inline Side-by-side
Showing with 214 additions and 193 deletions
src/sidebar/components/annotation-body.js
View file @ 96d9ff5e
import { Fragment, createElement } from 'preact';
import { createElement } from 'preact';
import { useState } from 'preact/hooks';
import propTypes from 'prop-types';
......@@ -8,6 +8,8 @@ import Button from './button';
import Excerpt from './excerpt';
import MarkdownEditor from './markdown-editor';
import MarkdownView from './markdown-view';
import TagEditor from './tag-editor';
import TagList from './tag-list';
/**
* Display the rendered content of an annotation.
......@@ -15,7 +17,9 @@ import MarkdownView from './markdown-view';
export default function AnnotationBody({
annotation,
isEditing,
onEditTags,
onEditText,
tags,
text,
}) {
// Should the text content of `Excerpt` be rendered in a collapsed state,
......@@ -31,36 +35,37 @@ export default function AnnotationBody({
? 'Show full annotation text'
: 'Show the first few lines only';
const showExcerpt = !isEditing && text.length > 0;
const showTagList = !isEditing && tags.length > 0;
return (
<Fragment>
<section className="annotation-body">
{!isEditing && (
<Excerpt
collapse={isCollapsed}
collapsedHeight={400}
inlineControls={false}
onCollapsibleChanged={setIsCollapsible}
onToggleCollapsed={setIsCollapsed}
overflowThreshold={20}
>
<MarkdownView
markdown={text}
textClass={{
'annotation-body__text': true,
'is-hidden': isHidden(annotation),
'has-content': text.length > 0,
}}
/>
</Excerpt>
)}
{isEditing && (
<MarkdownEditor
label="Annotation body"
text={text}
onEditText={onEditText}
<section className="annotation-body">
{showExcerpt && (
<Excerpt
collapse={isCollapsed}
collapsedHeight={400}
inlineControls={false}
onCollapsibleChanged={setIsCollapsible}
onToggleCollapsed={setIsCollapsed}
overflowThreshold={20}
>
<MarkdownView
markdown={text}
textClass={{
'annotation-body__text': true,
'is-hidden': isHidden(annotation),
'has-content': text.length > 0,
}}
/>
)}
</section>
</Excerpt>
)}
{isEditing && (
<MarkdownEditor
label="Annotation body"
text={text}
onEditText={onEditText}
/>
)}
{isCollapsible && !isEditing && (
<div className="annotation-body__collapse-toggle">
<Button
......@@ -71,7 +76,9 @@ export default function AnnotationBody({
/>
</div>
)}
</Fragment>
{showTagList && <TagList annotation={annotation} tags={tags} />}
{isEditing && <TagEditor onEditTags={onEditTags} tagList={tags} />}
</section>
);
}
......@@ -86,11 +93,18 @@ AnnotationBody.propTypes = {
*/
isEditing: propTypes.bool,
/**
* Callback invoked when the user edits tags.
*/
onEditTags: propTypes.func,
/**
* Callback invoked when the user edits the content of the annotation body.
*/
onEditText: propTypes.func,
tags: propTypes.array.isRequired,
/**
* The markdown annotation body, which is either rendered as HTML (if `isEditing`
* is false) or displayed in a text area otherwise.
......
src/sidebar/components/annotation.js
View file @ 96d9ff5e
......@@ -15,8 +15,6 @@ import AnnotationLicense from './annotation-license';
import AnnotationPublishControl from './annotation-publish-control';
import AnnotationQuote from './annotation-quote';
import Button from './button';
import TagEditor from './tag-editor';
import TagList from './tag-list';
/**
* A single annotation.
......@@ -52,7 +50,7 @@ function Annotation({
const toggleAction = threadIsCollapsed ? 'Show replies' : 'Hide replies';
const toggleText = `${toggleAction} (${replyCount})`;
const shouldShowActions = !isSaving && !isEditing && !isCollapsedReply;
const shouldShowActions = !isSaving && !isEditing;
const shouldShowLicense = isEditing && !isPrivate && group.type !== 'private';
const shouldShowReplyToggle = replyCount > 0 && !isReply(annotation);
......@@ -108,42 +106,52 @@ function Annotation({
showDocumentInfo={showDocumentInfo}
threadIsCollapsed={threadIsCollapsed}
/>
{hasQuote && <AnnotationQuote annotation={annotation} />}
<AnnotationBody
annotation={annotation}
isEditing={isEditing}
onEditText={onEditText}
text={text}
/>
{isEditing && <TagEditor onEditTags={onEditTags} tagList={tags} />}
{!isEditing && <TagList annotation={annotation} tags={tags} />}
<footer className="annotation__footer">
{isEditing && (
<div className="annotation__form-actions">
<AnnotationPublishControl
annotation={annotation}
isDisabled={isEmpty}
onSave={onSave}
/>
</div>
)}
{shouldShowLicense && <AnnotationLicense />}
<div className="annotation__controls">
{shouldShowReplyToggle && (
<Button
className="annotation__reply-toggle"
onClick={onToggleReplies}
buttonText={toggleText}
/>
)}
{isSaving && <div className="annotation__actions">Saving...</div>}
{shouldShowActions && (
<div className="annotation__actions">
<AnnotationActionBar annotation={annotation} onReply={onReply} />
{!isCollapsedReply && (
<AnnotationBody
annotation={annotation}
isEditing={isEditing}
onEditTags={onEditTags}
onEditText={onEditText}
tags={tags}
text={text}
/>
)}
{!isCollapsedReply && (
<footer className="annotation__footer">
{isEditing && (
<div className="annotation__form-actions">
<AnnotationPublishControl
annotation={annotation}
isDisabled={isEmpty}
onSave={onSave}
/>
</div>
)}
</div>
</footer>
{shouldShowLicense && <AnnotationLicense />}
<div className="annotation__controls">
{shouldShowReplyToggle && (
<Button
className="annotation__reply-toggle"
onClick={onToggleReplies}
buttonText={toggleText}
/>
)}
{isSaving && <div className="annotation__actions">Saving...</div>}
{shouldShowActions && (
<div className="annotation__actions">
<AnnotationActionBar
annotation={annotation}
onReply={onReply}
/>
</div>
)}
</div>
</footer>
)}
</article>
);
}
......
src/sidebar/components/test/annotation-body-test.js
View file @ 96d9ff5e
......@@ -16,6 +16,8 @@ describe('AnnotationBody', () => {
<AnnotationBody
annotation={fixtures.defaultAnnotation()}
isEditing={false}
onEditTags={() => null}
tags={[]}
text="test comment"
{...props}
/>
......@@ -88,10 +90,41 @@ describe('AnnotationBody', () => {
assert.equal(buttonProps.title, 'Show the first few lines only');
});
describe('tag list and editor', () => {
it('renders a list of tags if not editing and annotation has tags', () => {
const wrapper = createBody({ isEditing: false, tags: ['foo', 'bar'] });
assert.isTrue(wrapper.find('TagList').exists());
});
it('does not render a tag list if annotation has no tags', () => {
const wrapper = createBody({ isEditing: false, tags: [] });
assert.isFalse(wrapper.find('TagList').exists());
});
it('renders a tag editor if annotation is being edited', () => {
const wrapper = createBody({ isEditing: true, tags: ['foo', 'bar'] });
assert.isTrue(wrapper.find('TagEditor').exists());
assert.isFalse(wrapper.find('TagList').exists());
});
});
it(
'should pass a11y checks',
checkAccessibility({
content: () => createBody(),
})
checkAccessibility([
{
content: () => createBody(),
},
{
name: 'when annotation has tags (tag list)',
content: () => createBody({ isEditing: false, tags: ['foo', 'bar'] }),
},
{
name: 'when annotation is being edited and has tags',
content: () => createBody({ isEditing: true, tags: ['foo', 'bar'] }),
},
])
);
});
src/sidebar/components/test/annotation-test.js
View file @ 96d9ff5e
......@@ -8,8 +8,6 @@ import { checkAccessibility } from '../../../test-util/accessibility';
import mockImportedComponents from '../../../test-util/mock-imported-components';
import { waitFor } from '../../../test-util/wait';
// @TODO Note this import as `Annotation` for easier updating later
import Annotation from '../annotation';
import { $imports } from '../annotation';
......@@ -145,38 +143,6 @@ describe('Annotation', () => {
});
});
describe('tags', () => {
it('renders tag editor if `isEditing', () => {
setEditingMode(true);
const wrapper = createComponent();
assert.isTrue(wrapper.find('TagEditor').exists());
assert.isFalse(wrapper.find('TagList').exists());
});
it('updates annotation draft if tags changed', () => {
setEditingMode(true);
const wrapper = createComponent();
wrapper
.find('TagEditor')
.props()
.onEditTags({ tags: ['uno', 'dos'] });
const call = fakeStore.createDraft.getCall(0);
assert.calledOnce(fakeStore.createDraft);
assert.sameMembers(call.args[1].tags, ['uno', 'dos']);
});
it('renders tag list if not `isEditing', () => {
const wrapper = createComponent();
assert.isTrue(wrapper.find('TagList').exists());
assert.isFalse(wrapper.find('TagEditor').exists());
});
});
describe('publish control', () => {
it('should show the publish control if in edit mode', () => {
setEditingMode(true);
......@@ -461,28 +427,66 @@ describe('Annotation', () => {
assert.isFalse(wrapper.find('AnnotationActionBar').exists());
});
});
it('should not show annotations if the annotation is a collapsed reply', () => {
fakeMetadata.isReply.returns(true);
const wrapper = createComponent({ threadIsCollapsed: true });
context('annotation thread is collapsed', () => {
context('collapsed reply', () => {
beforeEach(() => {
fakeMetadata.isReply.returns(true);
});
assert.isFalse(wrapper.find('AnnotationActionBar').exists());
it('should not render body or footer', () => {
const wrapper = createComponent({ threadIsCollapsed: true });
assert.isFalse(wrapper.find('AnnotationBody').exists());
assert.isFalse(wrapper.find('footer').exists());
});
it('should not show actions', () => {
const wrapper = createComponent({ threadIsCollapsed: true });
assert.isFalse(wrapper.find('AnnotationActionBar').exists());
});
});
it(
'should pass a11y checks',
checkAccessibility([
{
content: () => createComponent(),
context('collapsed top-level annotation', () => {
it('should render body and footer', () => {
fakeMetadata.isReply.returns(false);
const wrapper = createComponent({ threadIsCollapsed: true });
assert.isTrue(wrapper.find('AnnotationBody').exists());
assert.isTrue(wrapper.find('footer').exists());
});
});
});
it(
'should pass a11y checks',
checkAccessibility([
{
content: () => createComponent(),
},
{
name: 'When editing',
content: () => {
setEditingMode(true);
return createComponent();
},
{
name: 'When editing',
content: () => {
setEditingMode(true);
return createComponent();
},
},
{
name: 'when a collapsed top-level thread',
content: () => {
fakeMetadata.isReply.returns(false);
return createComponent({ threadIsCollapsed: true });
},
])
);
});
},
{
name: 'when a collapsed reply',
content: () => {
fakeMetadata.isReply.returns(true);
return createComponent({ threadIsCollapsed: true });
},
},
])
);
});
src/styles/sidebar/components/annotation-body.scss
View file @ 96d9ff5e
......@@ -3,12 +3,7 @@
.annotation-body {
@include var.font-normal;
color: var.$grey-7;
// Margin between top of ascent of annotation body and
// bottom of ascent of annotation-quote should be ~15px.
margin-top: var.$layout-h-margin - 5px;
margin-bottom: var.$layout-h-margin;
margin-right: 0px;
margin-left: 0px;
margin: 1em 0;
}
.annotation-body__text {
......@@ -38,12 +33,12 @@
}
.annotation-body__collapse-toggle {
// Negative top margin to bring this up tight under `.annotation-body`
margin-top: -(var.$layout-h-margin - 5px);
margin: 0.5em 0;
display: flex;
justify-content: flex-end;
.annotation-body__collapse-toggle-button {
padding: 0.5em;
background-color: transparent;
}
}
src/styles/sidebar/components/annotation-header.scss
View file @ 96d9ff5e
......@@ -3,9 +3,6 @@
.annotation-header {
@include forms.pie-clearfix;
// Margin between top of x-height of username and
// top of the annotation card should be ~15px
margin-top: -(var.$layout-h-margin) + 10px;
color: var.$grey-6;
&__row {
......
src/styles/sidebar/components/annotation-license.scss
View file @ 96d9ff5e
......@@ -4,7 +4,7 @@
@include var.font-small;
border-top: 1px solid var.$grey-3;
padding-top: 0.5em;
margin: 0.5em 0;
margin: 1em 0;
&__link {
display: flex;
......
src/styles/sidebar/components/annotation-quote.scss
View file @ 96d9ff5e
@use '../../variables' as var;
.annotation-quote {
// Margin between top of ascent of annotation quote and
// bottom of ascent of username should be ~15px
margin-top: var.$layout-h-margin - 5px;
// Margin between bottom of ascent of annotation quote and
// top of ascent of annotation-body should be ~15px
margin-bottom: var.$layout-h-margin - 3px;
margin: 1em 0;
}
.annotation-quote.is-orphan {
......
src/styles/sidebar/components/annotation.scss
View file @ 96d9ff5e
......@@ -66,42 +66,7 @@
&__footer {
@include var.font-normal;
color: var.$grey-5;
margin-top: var.$layout-h-margin;
}
&--reply &__footer {
margin-top: var.$layout-h-margin - 8px;
}
}
// FIXME vertical rhythm here should be refactored
.annotation--reply {
.annotation-header {
// Margin between bottom of ascent of annotation card footer labels
// and top of ascent of username should be ~20px
margin-top: 0px;
}
.annotation-body {
// Margin between top of ascent of annotation body and
// bottom of ascent of username should be ~15px
margin-top: var.$layout-h-margin - 8px;
// Margin between bottom of ascent of annotation body and
// top of annotation footer labels should be ~15px
margin-bottom: var.$layout-h-margin - 3px;
}
}
.annotation--reply.is-collapsed {
margin-bottom: 0;
.annotation-header {
margin: 0;
}
.annotation-body,
.annotation-footer {
display: none;
margin-top: 1em;
}
}
......
src/styles/sidebar/components/tag-editor.scss
View file @ 96d9ff5e
......@@ -3,6 +3,8 @@
@use "../../variables" as var;
.tag-editor {
margin: 0.5em 0;
&__input {
@include forms.form-input;
width: 100%;
......@@ -11,12 +13,13 @@
&__tags {
display: flex;
flex-wrap: wrap;
margin: 0.5em 0;
}
&__item {
display: flex;
margin-right: 0.5em;
margin-bottom: 0.5em;
margin: 0.25em 0.5em 0.25em 0;
line-height: var.$base-line-height;
}
&__edit {
......
src/styles/sidebar/components/tag-list.scss
View file @ 96d9ff5e
@use "../../variables" as var;
.tag-list {
margin: 1em 0;
display: flex;
flex-wrap: wrap;
&__item {
display: flex;
margin-right: 0.5em;
margin-bottom: 0.5em;
margin: 0.25em 0.5em 0.25em 0;
padding: 0 0.5em;
line-height: var.$base-line-height;
background: var.$grey-1;
border: 1px solid var.$grey-3;
border-radius: 2px;
}
&__link,
&__text {
text-decoration: none;
color: var.$grey-6;
background: var.$grey-1;
border: 1px solid var.$grey-3;
border-radius: 2px;
padding: 0 0.5em;
}
&__link {
......
src/styles/sidebar/components/thread-list.scss
View file @ 96d9ff5e
......@@ -11,7 +11,7 @@
box-shadow: 0px 1px 1px 0px rgba(0, 0, 0, 0.1);
border-radius: 2px;
cursor: pointer;
padding: var.$layout-h-margin;
padding: 1em;
background-color: var.$white;
&:hover {
......
src/styles/sidebar/components/thread.scss
View file @ 96d9ff5e
......@@ -5,28 +5,35 @@
&--reply {
margin-top: 0.5em;
padding-top: 0.5em;
}
// Conserve space for deeper usable nesting
&__children {
margin-left: -0.75em;
}
// Left "channel" of thread
&__collapse {
margin: 0.25em;
margin-top: 0;
margin-right: 1em;
border-right: 1px dashed var.$grey-3;
// The entire channel is NOT clickable so don't make it look like it is
// (overrides `pointer` cursor applied to entire card)
cursor: auto;
border-left: 1px dashed var.$grey-3;
// Darken thread line on hover as a visual cue to show related thread items
&:hover {
border-left: 1px dashed var.$grey-4;
border-right: 1px dashed var.$grey-4;
}
.is-collapsed & {
border-left: none;
border-right: none;
}
}
// TODO These styles should be consolidated with other `Button` styles
&__collapse-button {
margin-left: -1.25em;
padding: 0.25em 0.75em 1em 0.75em;
margin-right: -1.25em;
padding: 0.25em 0.75em 0.75em 0.75em;
// Need a non-transparent background so that the dashed border line
// does not show through the button
background-color: var.$white;
......
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