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.

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

[#1267] Created generic logo component #1329

Merged

Conversation

stayprodio
Copy link
Contributor

No description provided.

Copy link
Contributor

@AudreyKj AudreyKj left a comment

Choose a reason for hiding this comment

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

well done! looks 🔥

@stayprodio stayprodio merged commit 3302c5f into develop Mar 22, 2021
@stayprodio stayprodio deleted the feature/1267-create-generic-sourceLogo-component branch March 22, 2021 15:14
Copy link
Contributor

@lucapette lucapette left a comment

Choose a reason for hiding this comment

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

I left a few minor comments.

I like the introduction of this component but I would simplify the API. The SourceLogo component doesn't need a channel, it needs a source and an optional

@@ -0,0 +1,56 @@
import React, {CSSProperties, SyntheticEvent} from 'react';
import {ReactComponent as GoogleLogo} from 'assets/images/icons/google_avatar.svg';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rename all of them so that they resemble as close as possible the file name as discussed with you and @AitorAlgorta last week.

In this case though, I would probably renamed the files :) as the components seem to have the right name?


const fallbackImageUrl = (event: SyntheticEvent<HTMLImageElement, Event>, source: string) => {
event.currentTarget.src = `https://s3.amazonaws.com/assets.airy.co/${source}_avatar.svg`;
event.currentTarget.alt = 'fallback image';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
event.currentTarget.alt = 'fallback image';
event.currentTarget.alt = `${source} fallback image`;


return (
<div className={styles.image} style={style}>
{channel?.metadata?.imageUrl || imageUrl ? getCustomLogo(channel) : getSourceLogo(channel)}
Copy link
Contributor

Choose a reason for hiding this comment

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

the channel can't be nil right? so channel? shouldn't be needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants