feat(lyrics-presenter): add 'Show more' button for playlists#81
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
XDMINT
force-pushed
the
feat/playlist-show-more
branch
from
2ebe85e to
397ed81
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
Hey @XDMINT, thanks for the PR. I'll have a look at this shortly |
Vija02
left a comment
Vija02
left a comment
There was a problem hiding this comment.
Thanks for this! A few things that needs to be changed but overall thanks for the PR. Let me know if you need any help
| export const ImportPlaylist = ({ | ||
| setSelectedPlaylistSongIds, | ||
| }: { | ||
| setSelectedPlaylistSongIds, |
There was a problem hiding this comment.
Looks like there's a formatting error here. Can you run prettier here?
| const { data: playlistData, isLoading } = | ||
| trpc.lyricsPresenter.playlist.useQuery(); | ||
| const [selectedPlaylistId, setSelectedPlaylistId] = useState<number | null>(null); | ||
| const [expandedIds, setExpandedIds] = useState<Set<number>>(new Set()); |
There was a problem hiding this comment.
Can you rename this into expandedPlaylistIds so that it's clear what id this is?
| const [expandedIds, setExpandedIds] = useState<Set<number>>(new Set()); | ||
| const { data: playlistData, isLoading } = trpc.lyricsPresenter.playlist.useQuery(); | ||
|
|
||
| const toggleExpand = (id: number) => { |
There was a problem hiding this comment.
Can we wrap this into a useCallback
| {!isExpanded && hiddenCount > 0 && ( | ||
| <p className="text-ellipsis overflow-hidden text-xs text-muted-foreground"> | ||
| …and {hiddenCount} more{" "} | ||
| <button |
There was a problem hiding this comment.
Let's use our button component from @repo/ui. There's the unstyled/link variant that you can use
|
|
||
| {isExpanded && allItems.length > MAX_VISIBLE && ( | ||
| <div className="mt-1"> | ||
| <button |
| </div> | ||
|
|
||
| {!isExpanded && hiddenCount > 0 && ( | ||
| <p className="text-ellipsis overflow-hidden text-xs text-muted-foreground"> |
There was a problem hiding this comment.
This muted-foreground color does not exist. Use secondary instead. You can see colors in theme.css
| <div className="mt-1"> | ||
| <button | ||
| type="button" | ||
| className="text-xs underline text-muted-foreground hover:no-underline" |
| ))} | ||
| </div> | ||
|
|
||
| {!isExpanded && hiddenCount > 0 && ( |
There was a problem hiding this comment.
For the styles here:
- The margin doesn't match as with when it's open. Can we add mt-1 here too
- Can we make the button have
pointeras a cursor please
…without IDE files
XDMINT
force-pushed
the
feat/playlist-show-more
branch
from
a8f05ba to
063890c
Compare
2 participants
Summary
Adds a compact playlist preview to the lyrics-presenter import modal:
@repo/ui(link variant)secondaryinstead ofmuted-foreground) to matchtheme.cssmt-1, pointer cursor for toggle button)expandedIds→expandedPlaylistIds)useCallbackMotivation
Improves readability and accessibility for long playlists.
Closes #76.
Implementation
ImportPlaylist.tsxaria-controls/aria-expanded@repo/uiensures consistent styling across the projectTesting
AUTO_LOGIN=1and mock/real playlists