From a8fcff5c14681afbe383f3d6612076317ed5861c Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Thu, 17 Dec 2020 15:45:55 -0500 Subject: [PATCH 1/6] Clear selection when a filter is set --- src/sidebar/store/modules/selection.js | 4 ++++ src/sidebar/store/modules/test/selection-test.js | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/sidebar/store/modules/selection.js b/src/sidebar/store/modules/selection.js index 6253ccc272d..866443fb976 100644 --- a/src/sidebar/store/modules/selection.js +++ b/src/sidebar/store/modules/selection.js @@ -156,6 +156,10 @@ const update = { return resetSelection(); }, + SET_FILTER: function () { + return { ...resetSelection(), expanded: {} }; + }, + SET_FILTER_QUERY: function () { return { ...resetSelection(), expanded: {} }; }, diff --git a/src/sidebar/store/modules/test/selection-test.js b/src/sidebar/store/modules/test/selection-test.js index 2de48526aba..a1292faa92c 100644 --- a/src/sidebar/store/modules/test/selection-test.js +++ b/src/sidebar/store/modules/test/selection-test.js @@ -159,6 +159,18 @@ describe('sidebar/store/modules/selection', () => { }); }); + describe('SET_FILTER', () => { + it('clears selection', () => { + store.selectAnnotations([1, 2, 3]); + store.setForcedVisible(2, true); + + store.setFilter('user', { value: 'dingbat', display: 'Ding Bat' }); + + assert.isEmpty(store.selectedAnnotations()); + assert.isEmpty(store.forcedVisibleAnnotations()); + }); + }); + describe('SET_FILTER_QUERY', () => { it('clears selection', () => { store.selectAnnotations([1, 2, 3]); From 1ad4004d8b430475c3583d8ed097a031c6746f2a Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Thu, 17 Dec 2020 15:46:25 -0500 Subject: [PATCH 2/6] Handle focus-user filter conflict when setting filters --- src/sidebar/store/modules/filters.js | 24 +++++++++-- .../store/modules/test/filters-test.js | 41 +++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/src/sidebar/store/modules/filters.js b/src/sidebar/store/modules/filters.js index 7887015fc98..e36e7eff551 100644 --- a/src/sidebar/store/modules/filters.js +++ b/src/sidebar/store/modules/filters.js @@ -170,10 +170,21 @@ function changeFocusModeUser(user) { * @param {FilterOption} filterOption */ function setFilter(filterName, filterOption) { - return { - type: actions.SET_FILTER, - filterName, - filterOption, + return (dispatch, getState) => { + // If there is a filter conflict with focusFilters, deactivate focus + // mode to prevent unintended collisions and let the new filter value + // take precedence. + if (getState().filters.focusFilters?.[filterName]) { + dispatch({ + type: actions.SET_FOCUS_MODE, + active: false, + }); + } + dispatch({ + type: actions.SET_FILTER, + filterName, + filterOption, + }); }; } @@ -263,6 +274,10 @@ const getFilterValues = createSelector( } ); +function getFocusFilters(state) { + return state.focusFilters; +} + /** * Are there currently any active (applied) filters? */ @@ -286,6 +301,7 @@ export default storeModule({ getFilter, getFilters, getFilterValues, + getFocusFilters, hasAppliedFilter, }, }); diff --git a/src/sidebar/store/modules/test/filters-test.js b/src/sidebar/store/modules/test/filters-test.js index 7c28e5ecb5e..ebcdb1208ef 100644 --- a/src/sidebar/store/modules/test/filters-test.js +++ b/src/sidebar/store/modules/test/filters-test.js @@ -74,6 +74,26 @@ describe('sidebar/store/modules/filters', () => { assert.isUndefined(filters.whatever); }); + it('disables focus mode if there is a conflicting filter key', () => { + store = createStore( + [filters], + [{ focus: { user: { username: 'somebody' } } }] + ); + + assert.isTrue(store.focusState().active); + + // No conflict in focusFilters on `elephant` + store.setFilter('elephant', { + value: 'pink', + display: 'Pink Elephant', + }); + + assert.isTrue(store.focusState().active); + + store.setFilter('user', { value: '', display: 'Everybody' }); + assert.isFalse(store.focusState().active); + }); + it('replaces pre-existing filter with the same key', () => { store.setFilter('whatever', { value: 'anyOldThing', @@ -251,6 +271,27 @@ describe('sidebar/store/modules/filters', () => { }); }); + describe('getFocusFilters', () => { + it('returns any set focus filters', () => { + store = createStore( + [filters], + [ + { + focus: { + user: { username: 'somebody', displayName: 'Ding Bat' }, + }, + }, + ] + ); + const focusFilters = store.getFocusFilters(); + assert.exists(focusFilters.user); + assert.deepEqual(focusFilters.user, { + value: 'somebody', + display: 'Ding Bat', + }); + }); + }); + describe('hasAppliedFilter', () => { it('returns true if there is a search query set', () => { store.setFilterQuery('foobar'); From b1a0b6e4431c0b9843f28732eee4a617a01f4afe Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Thu, 17 Dec 2020 15:48:13 -0500 Subject: [PATCH 3/6] Add NotebookResultCount component --- .../components/notebook-result-count.js | 55 ++++++++ src/sidebar/components/notebook-view.js | 21 +--- .../test/notebook-result-count-test.js | 118 ++++++++++++++++++ .../components/test/notebook-view-test.js | 33 +---- 4 files changed, 179 insertions(+), 48 deletions(-) create mode 100644 src/sidebar/components/notebook-result-count.js create mode 100644 src/sidebar/components/test/notebook-result-count-test.js diff --git a/src/sidebar/components/notebook-result-count.js b/src/sidebar/components/notebook-result-count.js new file mode 100644 index 00000000000..25e4ea79c1d --- /dev/null +++ b/src/sidebar/components/notebook-result-count.js @@ -0,0 +1,55 @@ +import { createElement } from 'preact'; + +import useRootThread from './hooks/use-root-thread'; +import { useStoreProxy } from '../store/use-store'; +import { countVisible } from '../util/thread'; + +/** + * Render count of annotations (or filtered results) visible in the notebook view + * + * There are three possible overall states: + * - No results (regardless of whether annotations are filtered): "No results" + * - Annotations are unfiltered: "X threads (Y annotations)" + * - Annotations are filtered: "X results [(and Y more)]" + */ +function NotebookResultCount() { + const store = useStoreProxy(); + const forcedVisibleCount = store.forcedVisibleAnnotations().length; + const hasAppliedFilter = store.hasAppliedFilter(); + const rootThread = useRootThread(); + const visibleCount = countVisible(rootThread); + + const hasResults = rootThread.children.length > 0; + + const hasForcedVisible = forcedVisibleCount > 0; + const matchCount = visibleCount - forcedVisibleCount; + const threadCount = rootThread.children.length; + + return ( + + {!hasResults &&

No results

} + {hasResults && hasAppliedFilter && ( + +

+ {matchCount} {matchCount === 1 ? 'result' : 'results'} +

+ {hasForcedVisible && (and {forcedVisibleCount} more)} + + )} + {hasResults && !hasAppliedFilter && ( + +

+ {threadCount} {threadCount === 1 ? 'thread' : 'threads'} +

{' '} + + ({visibleCount} {visibleCount === 1 ? 'annotation' : 'annotations'}) + + + )} + + ); +} + +NotebookResultCount.propTypes = {}; + +export default NotebookResultCount; diff --git a/src/sidebar/components/notebook-view.js b/src/sidebar/components/notebook-view.js index 8cd7aff568b..1f4ff35f87f 100644 --- a/src/sidebar/components/notebook-view.js +++ b/src/sidebar/components/notebook-view.js @@ -6,6 +6,7 @@ import { withServices } from '../util/service-context'; import useRootThread from './hooks/use-root-thread'; import { useStoreProxy } from '../store/use-store'; +import NotebookResultCount from './notebook-result-count'; import ThreadList from './thread-list'; /** @@ -38,31 +39,13 @@ function NotebookView({ loadAnnotationsService }) { const rootThread = useRootThread(); const groupName = focusedGroup?.name ?? '…'; - const hasResults = rootThread.totalChildren > 0; - const threadCount = - rootThread.totalChildren === 1 - ? '1 thread' - : `${rootThread.totalChildren} threads`; - const annotationCount = - rootThread.replyCount === 1 - ? '1 annotation' - : `${rootThread.replyCount} annotations`; - return (

{groupName}

- {hasResults ? ( - -

{threadCount}

({annotationCount}) - - ) : ( - -

No results

- - )} +
diff --git a/src/sidebar/components/test/notebook-result-count-test.js b/src/sidebar/components/test/notebook-result-count-test.js new file mode 100644 index 00000000000..39a44f364d8 --- /dev/null +++ b/src/sidebar/components/test/notebook-result-count-test.js @@ -0,0 +1,118 @@ +import { mount } from 'enzyme'; +import { createElement } from 'preact'; + +import NotebookResultCount from '../notebook-result-count'; +import { $imports } from '../notebook-result-count'; + +describe('NotebookResultCount', () => { + let fakeCountVisible; + let fakeUseRootThread; + let fakeStore; + + const createComponent = () => { + return mount(); + }; + + beforeEach(() => { + fakeCountVisible = sinon.stub().returns(0); + fakeUseRootThread = sinon.stub().returns({}); + + fakeStore = { + forcedVisibleAnnotations: sinon.stub().returns([]), + hasAppliedFilter: sinon.stub().returns(false), + }; + + $imports.$mock({ + './hooks/use-root-thread': fakeUseRootThread, + '../store/use-store': { useStoreProxy: () => fakeStore }, + '../util/thread': { countVisible: fakeCountVisible }, + }); + }); + + afterEach(() => { + $imports.$restore(); + }); + + context('when there are no results', () => { + it('should show "No Results" if no filters are applied', () => { + fakeStore.hasAppliedFilter.returns(false); + fakeUseRootThread.returns({ children: [] }); + + const wrapper = createComponent(); + + assert.equal(wrapper.text(), 'No results'); + }); + + it('should show "No Results" if filters are applied', () => { + fakeStore.hasAppliedFilter.returns(true); + fakeUseRootThread.returns({ children: [] }); + + const wrapper = createComponent(); + + assert.equal(wrapper.text(), 'No results'); + }); + }); + + context('no applied filter', () => { + [ + { + thread: { children: [1] }, + visibleCount: 1, + expected: '1 thread (1 annotation)', + }, + { + thread: { children: [1] }, + visibleCount: 2, + expected: '1 thread (2 annotations)', + }, + { + thread: { children: [1, 2] }, + visibleCount: 2, + expected: '2 threads (2 annotations)', + }, + ].forEach(test => { + it('should render a count of threads and annotations', () => { + fakeCountVisible.returns(test.visibleCount); + fakeUseRootThread.returns(test.thread); + + const wrapper = createComponent(); + + assert.equal(wrapper.text(), test.expected); + }); + }); + }); + + context('with one or more applied filters', () => { + [ + { + forcedVisible: [], + thread: { children: [1] }, + visibleCount: 1, + expected: '1 result', + }, + { + forcedVisible: [], + thread: { children: [1] }, + visibleCount: 2, + expected: '2 results', + }, + { + forcedVisible: [1], + thread: { children: [1] }, + visibleCount: 3, + expected: '2 results (and 1 more)', + }, + ].forEach(test => { + it('should render a count of results', () => { + fakeStore.hasAppliedFilter.returns(true); + fakeStore.forcedVisibleAnnotations.returns(test.forcedVisible); + fakeUseRootThread.returns(test.thread); + fakeCountVisible.returns(test.visibleCount); + + const wrapper = createComponent(); + + assert.equal(wrapper.text(), test.expected); + }); + }); + }); +}); diff --git a/src/sidebar/components/test/notebook-view-test.js b/src/sidebar/components/test/notebook-view-test.js index a17984fbd99..7d2914c2ce0 100644 --- a/src/sidebar/components/test/notebook-view-test.js +++ b/src/sidebar/components/test/notebook-view-test.js @@ -64,34 +64,9 @@ describe('NotebookView', () => { assert.equal(wrapper.find('.notebook-view__heading').text(), '…'); }); - describe('results count', () => { - [ - { - rootThread: { totalChildren: 5, replyCount: 15 }, - expected: '5 threads (15 annotations)', - }, - { - rootThread: { totalChildren: 0, replyCount: 0 }, - expected: 'No results', - }, - { - rootThread: { totalChildren: 0, replyCount: 15 }, - expected: 'No results', - }, - { - rootThread: { totalChildren: 1, replyCount: 1 }, - expected: '1 thread (1 annotation)', - }, - ].forEach(test => { - it('renders number of threads and annotations', () => { - fakeUseRootThread.returns(test.rootThread); - const wrapper = createComponent(); - - assert.equal( - wrapper.find('.notebook-view__results').text(), - test.expected - ); - }); - }); + it('renders results (counts)', () => { + const wrapper = createComponent(); + assert.isTrue(wrapper.find('NotebookResultCount').exists()); + }); }); }); From b4cdf482ca4f1ee4a1f09e939803817edc6982f9 Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Thu, 17 Dec 2020 15:48:37 -0500 Subject: [PATCH 4/6] Add `useUserFilterOptions` hook --- .../hooks/test/use-filter-options-test.js | 111 ++++++++++++++++++ .../components/hooks/use-filter-options.js | 51 ++++++++ 2 files changed, 162 insertions(+) create mode 100644 src/sidebar/components/hooks/test/use-filter-options-test.js create mode 100644 src/sidebar/components/hooks/use-filter-options.js diff --git a/src/sidebar/components/hooks/test/use-filter-options-test.js b/src/sidebar/components/hooks/test/use-filter-options-test.js new file mode 100644 index 00000000000..8d6da27515f --- /dev/null +++ b/src/sidebar/components/hooks/test/use-filter-options-test.js @@ -0,0 +1,111 @@ +import { mount } from 'enzyme'; +import { createElement } from 'preact'; + +import { useUserFilterOptions } from '../use-filter-options'; +import { $imports } from '../use-filter-options'; + +describe('sidebar/components/hooks/use-user-filter-options', () => { + let fakeStore; + let lastUserOptions; + + // Mount a dummy component to be able to use the hook + function DummyComponent() { + lastUserOptions = useUserFilterOptions(); + } + + function annotationFixtures() { + return [ + { + user: 'acct:dingbat@localhost', + user_info: { display_name: 'Ding Bat' }, + }, + { + user: 'acct:abalone@localhost', + user_info: { display_name: 'Aba Lone' }, + }, + { + user: 'acct:bananagram@localhost', + user_info: { display_name: 'Zerk' }, + }, + { + user: 'acct:dingbat@localhost', + user_info: { display_name: 'Ding Bat' }, + }, + ]; + } + + beforeEach(() => { + fakeStore = { + allAnnotations: sinon.stub().returns([]), + getFocusFilters: sinon.stub().returns({}), + isFeatureEnabled: sinon.stub().returns(false), + }; + + $imports.$mock({ + '../../store/use-store': { useStoreProxy: () => fakeStore }, + }); + }); + + afterEach(() => { + $imports.$restore(); + }); + + it('should return a user filter option for each user who has authored an annotation', () => { + fakeStore.allAnnotations.returns(annotationFixtures()); + + mount(); + + assert.deepEqual(lastUserOptions, [ + { value: 'abalone', display: 'abalone' }, + { value: 'bananagram', display: 'bananagram' }, + { value: 'dingbat', display: 'dingbat' }, + ]); + }); + + it('should use display names if feature flag enabled', () => { + fakeStore.allAnnotations.returns(annotationFixtures()); + fakeStore.isFeatureEnabled.withArgs('client_display_names').returns(true); + + mount(); + + assert.deepEqual(lastUserOptions, [ + { value: 'abalone', display: 'Aba Lone' }, + { value: 'dingbat', display: 'Ding Bat' }, + { value: 'bananagram', display: 'Zerk' }, + ]); + }); + + it('should add focused-user filter information if configured', () => { + fakeStore.allAnnotations.returns(annotationFixtures()); + fakeStore.isFeatureEnabled.withArgs('client_display_names').returns(true); + fakeStore.getFocusFilters.returns({ + user: { value: 'carrotNumberOne', display: 'Number One Carrot' }, + }); + + mount(); + + assert.deepEqual(lastUserOptions, [ + { value: 'abalone', display: 'Aba Lone' }, + { value: 'dingbat', display: 'Ding Bat' }, + { value: 'carrotNumberOne', display: 'Number One Carrot' }, + { value: 'bananagram', display: 'Zerk' }, + ]); + }); + + it('always uses display name for focused user', () => { + fakeStore.allAnnotations.returns(annotationFixtures()); + fakeStore.isFeatureEnabled.withArgs('client_display_names').returns(false); + fakeStore.getFocusFilters.returns({ + user: { value: 'carrotNumberOne', display: 'Numero Uno Zanahoria' }, + }); + + mount(); + + assert.deepEqual(lastUserOptions, [ + { value: 'abalone', display: 'abalone' }, + { value: 'bananagram', display: 'bananagram' }, + { value: 'dingbat', display: 'dingbat' }, + { value: 'carrotNumberOne', display: 'Numero Uno Zanahoria' }, + ]); + }); +}); diff --git a/src/sidebar/components/hooks/use-filter-options.js b/src/sidebar/components/hooks/use-filter-options.js new file mode 100644 index 00000000000..f8bd429ec62 --- /dev/null +++ b/src/sidebar/components/hooks/use-filter-options.js @@ -0,0 +1,51 @@ +import { useMemo } from 'preact/hooks'; + +import { useStoreProxy } from '../../store/use-store'; +import { username } from '../../util/account-id'; + +/** @typedef {import('../../store/modules/filters').FilterOption} FilterOption */ + +/** + * Generate a list of users for filtering annotations; update when set of + * annotations or filter state changes meaningfully. + * + * @return {FilterOption[]} + */ +export function useUserFilterOptions() { + const store = useStoreProxy(); + const annotations = store.allAnnotations(); + const focusFilters = store.getFocusFilters(); + const showDisplayNames = store.isFeatureEnabled('client_display_names'); + + return useMemo(() => { + // Determine unique users (authors) in annotation collection + const users = {}; + annotations.forEach(annotation => { + const username_ = username(annotation.user); + const displayValue = + showDisplayNames && annotation.user_info?.display_name + ? annotation.user_info.display_name + : username_; + users[username_] = displayValue; + }); + + // If user-focus is configured (even if not applied) add a filter + // option for that user. Note that this always respects the display + // value, even if `client_display_names` feature flags is not enabled: + // this matches current implementation of focus mode. + if (focusFilters.user) { + const username_ = + username(focusFilters.user.value) || focusFilters.user.value; + users[username_] = focusFilters.user.display; + } + + // Convert to `FilterOption` objects + const userOptions = Object.keys(users).map(user => { + return { display: users[user], value: user }; + }); + + userOptions.sort((a, b) => a.display.localeCompare(b.display)); + + return userOptions; + }, [annotations, focusFilters, showDisplayNames]); +} From ac6595b63b1c8a51c7fd8bbc93285b7f1971cc2e Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Thu, 17 Dec 2020 15:48:55 -0500 Subject: [PATCH 5/6] Add FilterSelect component --- src/sidebar/components/filter-select.js | 69 ++++++++++++++ .../components/test/filter-select-test.js | 92 +++++++++++++++++++ .../sidebar/components/filter-select.scss | 21 +++++ src/styles/sidebar/sidebar.scss | 1 + 4 files changed, 183 insertions(+) create mode 100644 src/sidebar/components/filter-select.js create mode 100644 src/sidebar/components/test/filter-select-test.js create mode 100644 src/styles/sidebar/components/filter-select.scss diff --git a/src/sidebar/components/filter-select.js b/src/sidebar/components/filter-select.js new file mode 100644 index 00000000000..d8f5ff7e0aa --- /dev/null +++ b/src/sidebar/components/filter-select.js @@ -0,0 +1,69 @@ +import { createElement } from 'preact'; +import propTypes from 'prop-types'; + +import Menu from './menu'; +import MenuItem from './menu-item'; +import SvgIcon from '../../shared/components/svg-icon'; + +/** + * @typedef {import('../store/modules/filters').FilterOption} FilterOption + */ + +/** + * @typedef FilterSelectProps + * @prop {FilterOption} defaultOption + * @prop {string} [icon] + * @prop {(selectedFilter: FilterOption) => any} onSelect + * @prop {FilterOption[]} options + * @prop {FilterOption} [selectedOption] + * @prop {string} title + */ + +/** + * A select-element-like control for selecting one of a defined set of + * options. + * + * @param {FilterSelectProps} props + */ +function FilterSelect({ + defaultOption, + icon, + onSelect, + options, + selectedOption, + title, +}) { + const filterOptions = [defaultOption, ...options]; + const selected = selectedOption ?? defaultOption; + + const menuLabel = ( + + {icon && } + {selected.display} + + ); + + return ( + + {filterOptions.map(filterOption => ( + onSelect(filterOption)} + key={filterOption.value} + isSelected={filterOption.value === selected.value} + label={filterOption.display} + /> + ))} + + ); +} + +FilterSelect.propTypes = { + defaultOption: propTypes.object, + icon: propTypes.string, + onSelect: propTypes.func, + options: propTypes.array, + selectedOption: propTypes.object, + title: propTypes.string, +}; + +export default FilterSelect; diff --git a/src/sidebar/components/test/filter-select-test.js b/src/sidebar/components/test/filter-select-test.js new file mode 100644 index 00000000000..f3430815c04 --- /dev/null +++ b/src/sidebar/components/test/filter-select-test.js @@ -0,0 +1,92 @@ +import { mount } from 'enzyme'; +import { createElement } from 'preact'; + +import FilterSelect from '../filter-select'; +import { $imports } from '../filter-select'; + +import mockImportedComponents from '../../../test-util/mock-imported-components'; + +describe('FilterSelect', () => { + let someOptions; + const createComponent = props => { + return mount( + null} + options={someOptions} + title="Select one" + {...props} + /> + ); + }; + + beforeEach(() => { + someOptions = [ + { value: 'onevalue', display: 'One Value' }, + { value: 'twovalue', display: 'Two Value' }, + ]; + $imports.$mock(mockImportedComponents()); + }); + + afterEach(() => { + $imports.$restore(); + }); + + it('should render option display values', () => { + const wrapper = createComponent(); + + const selectItems = wrapper.find('MenuItem'); + + assert.equal(selectItems.length, 3); + // First, the default option + assert.deepEqual(selectItems.at(0).props().label, 'all'); + // Then the other options + assert.deepEqual(selectItems.at(1).props().label, 'One Value'); + assert.deepEqual(selectItems.at(2).props().label, 'Two Value'); + }); + + it('should invoke `onSelect` callback when an option is selected', () => { + const fakeOnSelect = sinon.stub(); + + const wrapper = createComponent({ onSelect: fakeOnSelect }); + + const secondOption = wrapper.find('MenuItem').at(1); + secondOption.props().onClick(); + + assert.calledOnce(fakeOnSelect); + assert.calledWith( + fakeOnSelect, + sinon.match({ value: 'onevalue', display: 'One Value' }) + ); + }); + + it('should render provided icon and selected option in label', () => { + const wrapper = createComponent({ icon: 'profile' }); + + const label = mount(wrapper.find('Menu').props().label); + const icon = label.find('SvgIcon'); + + assert.isTrue(icon.exists()); + assert.equal(icon.props().name, 'profile'); + // Default option should be selected as we didn't indicate a selected option + assert.equal(label.text(), 'all'); + }); + + it('should render provided title', () => { + const wrapper = createComponent({ title: 'Select something' }); + + assert.equal(wrapper.find('Menu').props().title, 'Select something'); + }); + + it('should denote the selected option as selected', () => { + const wrapper = createComponent({ + selectedOption: { value: 'twovalue', display: 'Two Value' }, + }); + + const label = mount(wrapper.find('Menu').props().label); + + assert.equal(label.text(), 'Two Value'); + assert.isFalse(wrapper.find('MenuItem').at(1).props().isSelected); + assert.isTrue(wrapper.find('MenuItem').at(2).props().isSelected); + }); +}); diff --git a/src/styles/sidebar/components/filter-select.scss b/src/styles/sidebar/components/filter-select.scss new file mode 100644 index 00000000000..89902673a1c --- /dev/null +++ b/src/styles/sidebar/components/filter-select.scss @@ -0,0 +1,21 @@ +@use "../../mixins/utils"; +@use "../../variables" as var; + +.filter-select { + &__menu-label { + @include utils.font--large; + align-items: center; + color: var.$color-text; + display: flex; + + // Prevent label from wrapping if top bar is too narrow to fit all of its + // items. + flex-shrink: 0; + font-weight: bold; + } + + &__menu-icon { + @include utils.icon--medium; + margin-right: var.$layout-space--xsmall; + } +} diff --git a/src/styles/sidebar/sidebar.scss b/src/styles/sidebar/sidebar.scss index 788fe0258b8..dfbbf2692f3 100644 --- a/src/styles/sidebar/sidebar.scss +++ b/src/styles/sidebar/sidebar.scss @@ -26,6 +26,7 @@ @use './components/autocomplete-list'; @use './components/button'; @use './components/excerpt'; +@use './components/filter-select'; @use './components/filter-status'; @use './components/group-list'; @use './components/group-list-item'; From 2aaef5980061071dc0e9accd52eb6de601f9eb31 Mon Sep 17 00:00:00 2001 From: Lyza Danger Gardner Date: Fri, 18 Dec 2020 10:17:12 -0500 Subject: [PATCH 6/6] Add user filtering to Notebook --- src/sidebar/components/notebook-filters.js | 35 +++++++ src/sidebar/components/notebook-view.js | 4 + .../components/test/notebook-filters-test.js | 92 +++++++++++++++++++ .../components/test/notebook-view-test.js | 4 + .../sidebar/components/notebook-view.scss | 16 +++- 5 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 src/sidebar/components/notebook-filters.js create mode 100644 src/sidebar/components/test/notebook-filters-test.js diff --git a/src/sidebar/components/notebook-filters.js b/src/sidebar/components/notebook-filters.js new file mode 100644 index 00000000000..0d511d084c5 --- /dev/null +++ b/src/sidebar/components/notebook-filters.js @@ -0,0 +1,35 @@ +import { createElement } from 'preact'; + +import { useStoreProxy } from '../store/use-store'; +import { useUserFilterOptions } from './hooks/use-filter-options'; + +import FilterSelect from './filter-select'; + +/** + * @typedef {import('../store/modules/filters').FilterOption} FilterOption + */ + +/** + * Filters for the Notebook + */ +function NotebookFilters() { + const store = useStoreProxy(); + + const userFilter = store.getFilter('user'); + const userFilterOptions = useUserFilterOptions(); + + return ( + store.setFilter('user', userFilter)} + options={userFilterOptions} + selectedOption={userFilter} + title="Filter by user" + /> + ); +} + +NotebookFilters.propTypes = {}; + +export default NotebookFilters; diff --git a/src/sidebar/components/notebook-view.js b/src/sidebar/components/notebook-view.js index 1f4ff35f87f..a8ca5582e15 100644 --- a/src/sidebar/components/notebook-view.js +++ b/src/sidebar/components/notebook-view.js @@ -6,6 +6,7 @@ import { withServices } from '../util/service-context'; import useRootThread from './hooks/use-root-thread'; import { useStoreProxy } from '../store/use-store'; +import NotebookFilters from './notebook-filters'; import NotebookResultCount from './notebook-result-count'; import ThreadList from './thread-list'; @@ -44,6 +45,9 @@ function NotebookView({ loadAnnotationsService }) {

{groupName}

+
+ +
diff --git a/src/sidebar/components/test/notebook-filters-test.js b/src/sidebar/components/test/notebook-filters-test.js new file mode 100644 index 00000000000..df6cec6c17b --- /dev/null +++ b/src/sidebar/components/test/notebook-filters-test.js @@ -0,0 +1,92 @@ +import { mount } from 'enzyme'; +import { createElement } from 'preact'; + +import NotebookFilters from '../notebook-filters'; +import { $imports } from '../notebook-filters'; + +import mockImportedComponents from '../../../test-util/mock-imported-components'; + +describe('NotebookFilters', () => { + let fakeStore; + let fakeUseUserFilterOptions; + + const createComponent = () => { + return mount(); + }; + + beforeEach(() => { + fakeUseUserFilterOptions = sinon.stub().returns([]); + + fakeStore = { + getFilter: sinon.stub().returns(undefined), + setFilter: sinon.stub(), + }; + + $imports.$mock(mockImportedComponents()); + $imports.$mock({ + './hooks/use-filter-options': { + useUserFilterOptions: fakeUseUserFilterOptions, + }, + '../store/use-store': { useStoreProxy: () => fakeStore }, + }); + }); + + afterEach(() => { + $imports.$restore(); + }); + + it('should render a user filter with options', () => { + fakeUseUserFilterOptions.returns([ + { display: 'One User', value: 'oneuser' }, + ]); + + const wrapper = createComponent(); + + const props = wrapper.find('FilterSelect').props(); + assert.deepEqual(props.options[0], { + display: 'One User', + value: 'oneuser', + }); + assert.deepEqual(props.defaultOption, { value: '', display: 'Everybody' }); + assert.equal(props.icon, 'profile'); + assert.equal(props.title, 'Filter by user'); + assert.equal(props.options.length, 1); + assert.isUndefined(props.selectedOption); + }); + + it('should render the filter with a selected option if a user filter is applied', () => { + fakeUseUserFilterOptions.returns([ + { display: 'One User', value: 'oneuser' }, + ]); + fakeStore.getFilter + .withArgs('user') + .returns({ display: 'One User', value: 'oneuser' }); + + const wrapper = createComponent(); + + assert.deepEqual(wrapper.find('FilterSelect').props().selectedOption, { + display: 'One User', + value: 'oneuser', + }); + }); + + it('should set a user filter when a user is selected', () => { + fakeUseUserFilterOptions.returns([ + { display: 'One User', value: 'oneuser' }, + ]); + + const wrapper = createComponent(); + + wrapper + .find('FilterSelect') + .props() + .onSelect({ display: 'One User', value: 'oneuser' }); + + assert.calledOnce(fakeStore.setFilter); + assert.calledWith( + fakeStore.setFilter, + 'user', + sinon.match({ display: 'One User', value: 'oneuser' }) + ); + }); +}); diff --git a/src/sidebar/components/test/notebook-view-test.js b/src/sidebar/components/test/notebook-view-test.js index 7d2914c2ce0..5a9fbe2a978 100644 --- a/src/sidebar/components/test/notebook-view-test.js +++ b/src/sidebar/components/test/notebook-view-test.js @@ -68,5 +68,9 @@ describe('NotebookView', () => { const wrapper = createComponent(); assert.isTrue(wrapper.find('NotebookResultCount').exists()); }); + + it('renders filters', () => { + const wrapper = createComponent(); + assert.isTrue(wrapper.find('NotebookFilters').exists()); }); }); diff --git a/src/styles/sidebar/components/notebook-view.scss b/src/styles/sidebar/components/notebook-view.scss index 3a218aa44bb..0764017267d 100644 --- a/src/styles/sidebar/components/notebook-view.scss +++ b/src/styles/sidebar/components/notebook-view.scss @@ -10,6 +10,7 @@ 'results' 'items'; + /* TODO: find a better place for these heading styles */ h1 { display: inline; font-size: var.$font-size--heading; @@ -28,6 +29,10 @@ font-weight: bold; } + &__filters { + grid-area: filters; + } + &__results { grid-area: results; } @@ -38,13 +43,16 @@ @include responsive.tablet-and-up { grid-template-areas: - 'heading results' + 'heading heading' + 'filters results' 'items items'; + &__filters { + justify-self: start; + } + &__results { - // Right-aligned when sharing a row with the heading - text-align: right; - align-self: end; + justify-self: end; } } }