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.

Sign up for GitHub

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

Added tagged passage and story count display to Passage and Story Tags Dialogs #1498

Closed
wants to merge 1 commit into from
Closed

Added tagged passage and story count display to Passage and Story Tags Dialogs #1498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/components/tag/__mocks__/tag-editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ export const TagEditor: React.FC<TagEditorProps> = props => (
<button onClick={() => props.onChangeName('mock-new-name')}>
onChangeName
</button>
<span>{`${props.count} ${props.count > 1 ? 'passages' : 'passage'}`}</span>
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is a mock, let's just do

<span data-testid="count">{props.count}</span>

</div>
);
9 changes: 9 additions & 0 deletions src/components/tag/__tests__/tag-editor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import {TagEditor, TagEditorProps} from '../tag-editor';
describe('<TagEditor>', () => {
async function renderComponent(props?: Partial<TagEditorProps>) {
const tagName = lorem.word();
const mockCount = 1;
const result = render(
<TagEditor
allTags={[tagName]}
name={tagName}
count={mockCount}
onChangeColor={jest.fn()}
onChangeName={jest.fn()}
{...props}
Expand All @@ -25,6 +27,13 @@ describe('<TagEditor>', () => {
await renderComponent({name: 'test name'});
expect(screen.getByText('test name')).toBeInTheDocument();
});
it('displays the frequency of the tag', async() => {
await renderComponent({ count: 1});
expect(screen.getByText('1 passage')).toBeInTheDocument();

await renderComponent({ count: 2});
expect(screen.getByText('2 passages')).toBeInTheDocument();
})

it('calls the onChangeColor prop when the color is changed', async () => {
const onChangeColor = jest.fn();
Expand Down
6 changes: 5 additions & 1 deletion src/components/tag/tag-editor.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
.tag-editor {
display: grid;
grid-gap: var(--grid-size);
grid-template-columns: repeat(3, min-content);
grid-template-columns: repeat(4, min-content);
align-items: center;
}
.tag-count{
white-space: nowrap;
}

.tag-editor .tag-name {
Expand Down
4 changes: 3 additions & 1 deletion src/components/tag/tag-editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ export interface TagEditorProps {
allTags: string[];
color?: Color;
name: string;
count: number;
onChangeColor: (color: Color) => void;
onChangeName: (name: string) => void;
}

export const TagEditor: React.FC<TagEditorProps> = props => {
const {allTags, color, name, onChangeColor, onChangeName} = props;
const {allTags, color, name, count, onChangeColor, onChangeName} = props;
const [newName, setNewName] = React.useState(name);
const {t} = useTranslation();

Expand Down Expand Up @@ -52,6 +53,7 @@ export const TagEditor: React.FC<TagEditorProps> = props => {
>
{t('common.color')}
</TextSelect>
<span className="tag-count">{`${count} ${count > 1 ? 'passages' : 'passage'}`}</span>
Copy link
Owner

Choose a reason for hiding this comment

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

This text needs to be localized instead of hardcoding English. Take a look at StoryListRoute to see an example of how it handles plurals in the title. Putting just an English translation of the text in this PR is fine; I notify translators before a release so they can update the text.

I'd suggest using the key components.tagEditor.passageCount for this.

Tests will need to be updated too--we don't need to test that the localization works correctly, just that the right keys are displayed.

</div>
);
};
12 changes: 12 additions & 0 deletions src/dialogs/__tests__/passage-tags.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,18 @@ describe('<PassageTagsDialog>', () => {
expect(screen.getByTestId('mock-tag-editor-mock-tag2')).toBeInTheDocument();
});

it ('shows correct tag frequencies for every passage tags', async () => {
const story = fakeStory(2);

story.passages[0].tags = ['mock-tag', 'mock-tag2'];
story.passages[1].tags = ['mock-tag'];

await renderComponent({storyId: story.id}, {stories: [story]})

expect(within(screen.getByTestId('mock-tag-editor-mock-tag')).getByText(`2 passages`)).toBeInTheDocument();
; expect(within(screen.getByTestId('mock-tag-editor-mock-tag2')).getByText(`1 passage`)).toBeInTheDocument();
});

it('dispatches a story action if a tag is renamed', async () => {
const dispatch = jest.fn();
const story = fakeStory(1);
Expand Down
17 changes: 17 additions & 0 deletions src/dialogs/__tests__/story-tags.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,23 @@ describe('<StoryTagsDialog>', () => {
expect(screen.getByTestId('mock-tag-editor-mock-tag2')).toBeInTheDocument();
});

it ('renders tag editor with correct count for each tag', async () => {
const story1 = fakeStory();
const story2 = fakeStory();

story1.tags = ['tag1', 'tag2'];
story2.tags = ['tag2', 'tag3'];

await renderComponent(
{},
{stories: [story1, story2]}
);

expect(within(screen.getByTestId('mock-tag-editor-tag1')).getByText(`1 passage`)).toBeInTheDocument();
; expect(within(screen.getByTestId('mock-tag-editor-tag2')).getByText(`2 passages`)).toBeInTheDocument();
expect(within(screen.getByTestId('mock-tag-editor-tag3')).getByText(`1 passage`)).toBeInTheDocument();
});

it('dispatches a story action if a tag is renamed', async () => {
const dispatch = jest.fn();
const story = fakeStory();
Expand Down
6 changes: 4 additions & 2 deletions src/dialogs/passage-tags.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import {
setTagColor,
storyWithId,
renamePassageTag,
storyPassageTags
storyPassageTags,
passageTagFrequencies
} from '../store/stories';
import {useUndoableStoriesContext} from '../store/undoable-stories';
import {Color} from '../util/color';
Expand All @@ -24,7 +25,7 @@ export const PassageTagsDialog: React.FC<PassageTagsDialogProps> = props => {

const story = storyWithId(stories, storyId);
const tags = storyPassageTags(story);

const tagCountsInStory = passageTagFrequencies(story);
Copy link
Owner

Choose a reason for hiding this comment

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

This should be memoized for performance reasons.

function handleChangeColor(tagName: string, color: Color) {
dispatch(
setTagColor(story, tagName, color),
Expand Down Expand Up @@ -54,6 +55,7 @@ export const PassageTagsDialog: React.FC<PassageTagsDialogProps> = props => {
color={story.tagColors[tag]}
key={tag}
name={tag}
count={tagCountsInStory[tag]}
onChangeColor={color => handleChangeColor(tag, color)}
onChangeName={newName => handleChangeTagName(tag, newName)}
/>
Expand Down
4 changes: 3 additions & 1 deletion src/dialogs/story-tags.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {useTranslation} from 'react-i18next';
import {DialogCard} from '../components/container/dialog-card';
import {CardContent} from '../components/container/card';
import {DialogComponentProps} from './dialogs.types';
import {renameStoryTag, storyTags} from '../store/stories';
import {renameStoryTag, storyTags, storyTagFrequencies} from '../store/stories';
import {setPref, usePrefsContext} from '../store/prefs';
import {useUndoableStoriesContext} from '../store/undoable-stories';
import {Color} from '../util/color';
Expand All @@ -17,6 +17,7 @@ export const StoryTagsDialog: React.FC<StoryTagsDialogProps> = props => {
const {t} = useTranslation();

const tags = storyTags(stories);
const tagCountsInStories = storyTagFrequencies(stories);
Copy link
Owner

Choose a reason for hiding this comment

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

This also should be memoized.


function handleChangeColor(tagName: string, color: Color) {
prefsDispatch(
Expand All @@ -43,6 +44,7 @@ export const StoryTagsDialog: React.FC<StoryTagsDialogProps> = props => {
color={prefs.storyTagColors[tag]}
key={tag}
name={tag}
count={tagCountsInStories[tag]}
onChangeColor={color => handleChangeColor(tag, color)}
onChangeName={newName => handleChangeTagName(tag, newName)}
/>
Expand Down
53 changes: 52 additions & 1 deletion src/store/stories/__tests__/getters.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import {
storyWithName,
storyTags,
passagesMatchingSearch,
passagesMatchingFuzzySearch
passagesMatchingFuzzySearch,
passageTagFrequencies,
storyTagFrequencies
} from '../getters';
import {Passage, Story} from '../stories.types';
import {fakePassage, fakeStory} from '../../../test-util';
Expand Down Expand Up @@ -275,6 +277,55 @@ describe('storyPassageTags()', () => {
});
});

describe('passageTagFrequencies()', () => {
it('returns an object with correct counts of frequencies of tags across all passages in a story', () =>{
const story = fakeStory(3);
story.passages[0].tags = ['tag1', 'tag2'];
story.passages[1].tags = ['tag2', 'tag3'];
story.passages[2].tags = ['tag1'];

const expectedTagFrequencies ={
tag1: 2,
tag2: 2,
tag3: 1,
};

expect(passageTagFrequencies(story)).toEqual(expectedTagFrequencies);
});

it('returns empty object if no tags present', () => {
const story = fakeStory(2);
const expectedTagFrequencies ={};

expect(passageTagFrequencies(story)).toEqual(expectedTagFrequencies);
})

});

describe('storyTagFrequencies()', () => {
it('returns correct frequencies of tags of all stories', () => {
const stories = [fakeStory(), fakeStory()];

stories[0].tags = ['c', 'a'];
stories[1].tags = ['a', 'b'];

const expectedStoryTagFrequencies ={
a: 2,
b: 1,
c: 1,
};

expect(storyTagFrequencies(stories)).toEqual(expectedStoryTagFrequencies);
});

it('returns empty object with no tags', () => {
const stories = [fakeStory(), fakeStory()];
const expectedStoryTagFrequencies = {};

expect(storyTagFrequencies(stories)).toEqual(expectedStoryTagFrequencies);
});
});

describe('storyTags()', () => {
it('returns a sorted array of unique tags across stories', () => {
const stories = [fakeStory(), fakeStory()];
Expand Down
30 changes: 30 additions & 0 deletions src/store/stories/getters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,22 @@ export function storyPassageTags(story: Story) {
).sort();
}

export function passageTagFrequencies(story: Story) {
const tagPassageCounts: { [key: string]: number } = {};

story.passages.forEach(passage => {
if (passage.tags) {
passage.tags.forEach(tag=>{
if (!tagPassageCounts[tag]) {
tagPassageCounts[tag] = 1;
} else {
tagPassageCounts[tag] += 1;
}
})};
});
return tagPassageCounts;
}

Comment on lines +162 to +177
Copy link
Owner

Choose a reason for hiding this comment

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

More idiomatically (and to parallel the name with storyPassageTags above it):

export function storyPassageTagFrequencies(story: Story) {
	const counts: Record<string, number> = {};

	for (const passage of story.passages) {
		for (const tag of passage.tags) {
			counts[tag] = (counts[tag] ?? 0) + 1;
		}
	}

	return counts;
}

I'm not sure why you do a check on passage.tags not being defined--it should always be, but did you find that wasn't the case in practice?

export function storyStats(story: Story) {
const links = story.passages.reduce<string[]>(
(links, passage) => [
Expand Down Expand Up @@ -196,6 +212,20 @@ export function storyTags(stories: Story[]) {
).sort();
}

export function storyTagFrequencies(stories: Story[]) {
Copy link
Owner

Choose a reason for hiding this comment

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

Same suggestion as with passageTagFrequencies.

const tagFrequencies: {[key: string]: number} = {};

stories.forEach(story => {
if (story.tags) {
story.tags.forEach(tag => {
tagFrequencies[tag] = (tagFrequencies[tag] || 0) + 1;
});
}
});

return tagFrequencies;
}

export function storyWithId(stories: Story[], storyId: string) {
const result = stories.find(s => s.id === storyId);

Expand Down
Loading