-
Notifications
You must be signed in to change notification settings - Fork 50
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.
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
Sidebar show type #590
Sidebar show type #590
Conversation
@cosmicpotato137 Let's remove 'By show title' for now until the backend returns the hosts name. |
2b2a1cb
to
e40d4d5
Compare
pushed prettier changes to this commit e40d4d5 |
{Boolean(showTitle) && type !== 'show' && <Title>{showTitle}</Title>} | ||
{Boolean(showTitle) && type === 'show' && <ShowTitle>{showTitle}</ShowTitle>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure we can merge these two into one ternary operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we even need new component for that, we can use something like className={clsx({'is-show': type === 'show'})}
take a look to Avatar component, you should add default image there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look to my comments, feel free to ping me, if anything is not clear
{Boolean(showTitle) && type !== 'show' && <Title>{showTitle}</Title>} | ||
{Boolean(showTitle) && type === 'show' && <ShowTitle>{showTitle}</ShowTitle>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we even need new component for that, we can use something like className={clsx({'is-show': type === 'show'})}
@@ -102,16 +102,19 @@ export const Episode = ({ | |||
) : null} | |||
</Flex> | |||
|
|||
<Description data-testid="episode-description">{episodeTitle}</Description> | |||
{type !== 'show' && <Description data-testid="episode-description">{episodeTitle}</Description>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should pass showTitle here, and get rid of ShowTitle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what you mean by this. Should I just revert this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…-nav-fiber into sidebar-show-type
@Rassl finished updating based on your review in case you hadn't already :) |
@@ -86,7 +86,7 @@ export const Episode = ({ | |||
<Flex direction="row"> | |||
{!isSelectedView && ( | |||
<Flex align="center" pr={16}> | |||
<Avatar size={64} src={imageUrl} type={type || ''} /> | |||
<Avatar size={64} src={imageUrl !== '' ? imageUrl : } type={type || ''} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is supposed to be after :
?
you should change not here, and inside Avatar component, just add new type 'show'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
closes #546
I am having trouble with getting the default show cover image to appear. Is there something that needs to be done on the back end to return a node with the default png if it doesn't have one? And if so, where does that happen? Also, I couldn't find any show producer name, so I'm just using the name of the show for now.
For now, the UI styling is done!