Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/harmony/src/foundations/spacing/spacing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const spacing = {
}

export const iconSizes = {
'2xs': 12,
xs: 14,
s: 16,
m: 20,
Expand Down
119 changes: 87 additions & 32 deletions packages/web/src/components/collection/CollectionCard.test.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { SquareSizes } from '@audius/common/models'
import { Text } from '@audius/harmony'
import { merge } from 'lodash'
import { Routes, Route } from 'react-router-dom-v5-compat'
import { describe, it, expect } from 'vitest'

import { render, screen } from 'test/test-utils'
import { RenderOptions, render, screen } from 'test/test-utils'

import { CollectionCard } from './CollectionCard'

function renderCollectionCard() {
function renderCollectionCard(options?: RenderOptions) {
return render(
<Routes>
<Route path='/' element={<CollectionCard id={1} size='s' />} />
Expand All @@ -20,42 +21,45 @@ function renderCollectionCard() {
element={<Text variant='heading'>Test User Page</Text>}
/>
</Routes>,
{
reduxState: {
collections: {
entries: {
1: {
metadata: {
playlist_id: 1,
playlist_name: 'Test Collection',
playlist_owner_id: 2,
permalink: '/test-user/test-collection',
repost_count: 10,
save_count: 5,
_cover_art_sizes: {
[SquareSizes.SIZE_150_BY_150]: 'image-small.jpg',
[SquareSizes.SIZE_480_BY_480]: 'image-medium.jpg'
merge(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

{
reduxState: {
collections: {
entries: {
1: {
metadata: {
playlist_id: 1,
playlist_name: 'Test Collection',
playlist_owner_id: 2,
permalink: '/test-user/test-collection',
repost_count: 10,
save_count: 5,
_cover_art_sizes: {
[SquareSizes.SIZE_150_BY_150]: 'image-small.jpg',
[SquareSizes.SIZE_480_BY_480]: 'image-medium.jpg'
}
}
}
}
}
},
users: {
entries: {
2: { metadata: { handle: 'test-user', name: 'Test User' } }
},
users: {
entries: {
2: { metadata: { handle: 'test-user', name: 'Test User' } }
}
}
}
}
}
},
options
)
)
}

describe('CollectionCard', () => {
it('renders a button with the label comprising the collection and artist name', () => {
it('renders a button with the label comprising the collection and artist name, and favorites and reposts', () => {
renderCollectionCard()
expect(
screen.getByRole('button', {
name: /test collection test user/i
name: /test collection test user reposts 10 favorites 5/i

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want the reposts and favorites to read out as part of the overall card button label, or only if you drill down into the stats?

Just double checking since I know we made this change when we were playing around with label implementations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah i could see not including it for the top level label, but i think including it is nice for now? ill see if spotify has any pressables with stats on it, and see how they handle that. one negative with including it, is it takes a while to saw "available for purchase" which is probably a more important aspect?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Man I wish there was a way to interpolate the aria-labelledby to inject stuff like test collection **by** test user **with** but I dont think this is possible without just adding those directly into the labels I guess

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah agreed, fwiw this pattern is what spotify uses, it doesn't have the "by" etc

})
).toBeInTheDocument()
})
Expand Down Expand Up @@ -89,12 +93,63 @@ describe('CollectionCard', () => {
).toBeInTheDocument()
})

it('shows the number of reposts and favorites with the correct screen-reader text', () => {
renderCollectionCard()
expect(screen.getByTitle('Reposts')).toBeInTheDocument()
expect(screen.getByText('10')).toBeInTheDocument()
it('hidden collections are rendered correctly', () => {
renderCollectionCard({
reduxState: {
collections: { entries: { 1: { metadata: { is_private: true } } } }
}
Comment on lines +97 to +100

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this pattern. Very clean per test

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks! i wonder also if we can add helpers for quickly adding users, tracks collections, so you dont actually need to know the full redux shape.

})

expect(
screen.getByRole('button', { name: /test collection test user hidden/i })
).toBeInTheDocument()
})

it('premium locked collections are rendered correctly', () => {
renderCollectionCard({
reduxState: {
collections: {
entries: {
1: {
metadata: {
access: { stream: false },
stream_conditions: {
usdc_purchase: { price: 10, albumTrackPrice: 1, splits: {} }
}
}
}
}
}
}
})
Comment on lines +118 to +124

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lol ok maybe this one is slightly less clean, but not your fault

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah maybe we consider the {users: [{...}], tracks: [{...}]} pattern soon

expect(
screen.getByRole('button', {
name: /test collection test user reposts 10 favorites 5 available for purchase/i
})
)
})

expect(screen.getByTitle('Favorites')).toBeInTheDocument()
expect(screen.getByText('5')).toBeInTheDocument()
it('premium unlocked collections are rendered correctly', () => {
renderCollectionCard({
reduxState: {
collections: {
entries: {
1: {
metadata: {
access: { stream: true },
stream_conditions: {
usdc_purchase: { price: 10, albumTrackPrice: 1, splits: {} }
}
}
}
}
}
}
})
expect(
screen.getByRole('button', {
name: /test collection test user reposts 10 favorites 5 purchased/i
})
)
})
})
102 changes: 80 additions & 22 deletions packages/web/src/components/collection/CollectionCard.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
import { ID, SquareSizes } from '@audius/common/models'
import {
DogEarType,
ID,
SquareSizes,
isContentUSDCPurchaseGated
} from '@audius/common/models'
import { cacheCollectionsSelectors } from '@audius/common/store'
import { Divider, Flex, Paper, Text } from '@audius/harmony'
import IconHeart from '@audius/harmony/src/assets/icons/Heart.svg'
import IconRepost from '@audius/harmony/src/assets/icons/Repost.svg'
import { Link, useLinkClickHandler } from 'react-router-dom-v5-compat'

import { DogEar } from 'components/dog-ear'
import { UserLink } from 'components/link'
import { LockedStatusPill } from 'components/locked-status-pill'
import { useSelector } from 'utils/reducer'

import { CollectionImage } from './CollectionImage'
const { getCollection } = cacheCollectionsSelectors

const messages = {
repost: 'Reposts',
favorites: 'Favorites'
favorites: 'Favorites',
hidden: 'Hidden'
}

type CardSize = 's' | 'm' | 'l'
Expand Down Expand Up @@ -51,50 +59,100 @@ export const CollectionCard = (props: CollectionCardProps) => {
permalink,
playlist_owner_id,
repost_count,
save_count
save_count,
is_private,
access,
stream_conditions
} = collection

const isPurchase = isContentUSDCPurchaseGated(stream_conditions)

const dogEarType = is_private
? DogEarType.HIDDEN
: isPurchase && !access.stream
? DogEarType.USDC_PURCHASE
: null

return (
<Paper
role='button'
tabIndex={0}
onClick={handleClick}
aria-labelledby={`${id}-title ${id}-artist`}
aria-labelledby={`${id}-title ${id}-artist ${id}-repost ${id}-favorite ${id}-condition`}
direction='column'
border='default'
w={cardSizes[size]}
css={{ cursor: 'pointer' }}
css={{ cursor: 'pointer', overflow: 'unset' }}
>
{dogEarType ? (
<DogEar type={dogEarType} css={{ top: -1, left: -1 }} />
) : null}
<Flex direction='column' p='s' gap='s'>
<CollectionImage
collectionId={id}
size={cardSizeToCoverArtSizeMap[size]}
data-testid={`${id}-cover-art`}
/>
<Link id={`${id}-title`} to={permalink} css={{ alignSelf: 'center' }}>
<Text variant='title' tag='span' color='default' textAlign='center'>
<Text
id={`${id}-title`}
variant='title'
color='default'
ellipses
asChild
>
<Link to={permalink} css={{ pointerEvents: 'none' }}>
{playlist_name}
</Text>
</Link>
</Link>
</Text>
Comment on lines +96 to +106

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there not a CollectionLink component for this? Or is this one different from the standard links?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good question! we could use collection-link and pass the pointer-events none to it. that's probably the move honestly. collection link doesn't exist currently though, but ill add one as i start adding collection cards in

<UserLink
id={`${id}-artist`}
userId={playlist_owner_id}
css={{ alignSelf: 'center' }}
textVariant='body'
css={{ justifyContent: 'center' }}
/>
</Flex>
<Divider orientation='horizontal' />
<Flex gap='l' p='s' justifyContent='center'>
<Flex gap='xs' alignItems='center'>
<IconRepost size='s' color='subdued' title={messages.repost} />
<Text color='subdued' variant='body' size='s'>
{repost_count}
</Text>
</Flex>
<Flex gap='xs' alignItems='center'>
<IconHeart size='s' color='subdued' title={messages.favorites} />{' '}
<Text color='subdued' variant='body' size='s'>
{save_count}
<Flex
gap='l'
p='s'
justifyContent='center'
backgroundColor='surface1'
borderBottomLeftRadius='m'
borderBottomRightRadius='m'
>
{is_private ? (
<Text
variant='body'
size='s'
strength='strong'
color='subdued'
id={`${id}-condition`}
>
{messages.hidden}
</Text>
</Flex>
) : (
<>
<Flex gap='xs' alignItems='center' id={`${id}-repost`}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Very optional but I think you could make the aria text a slight bit better by adding this to this element

aria-label={`{repost_count} {messages.repost}`}

so that it reads out as 10 reposts instead of reposts 10

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(same for favorites)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah we had actually discussed this. 10 reposts reads like english, but reposts 10 contextualizes the number better by the time you hear it. Unsure which is canonical for accessibility

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah im down to investigate more on this. personally i also like that we can use the actual elements labels rather than have to add a custom label, in the aria-label case.

<IconRepost size='s' color='subdued' title={messages.repost} />
<Text variant='label' color='subdued'>
{repost_count}
</Text>
</Flex>
<Flex gap='xs' alignItems='center' id={`${id}-favorite`}>
<IconHeart size='s' color='subdued' title={messages.favorites} />
<Text variant='label' color='subdued'>
{save_count}
</Text>
</Flex>
{isPurchase ? (
<LockedStatusPill
variant='premium'
locked={!access.stream}
id={`${id}-condition`}
/>
) : null}
</>
)}
</Flex>
</Paper>
)
Expand Down
49 changes: 0 additions & 49 deletions packages/web/src/components/dog-ear/DogEar.module.css

This file was deleted.

Loading