Use the same component to render every app icon #655
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import React from 'react' | ||
| import PropTypes from 'prop-types' | ||
| import { appIconUrl } from '../../utils' | ||
| import RemoteImage from '../RemoteImage' | ||
|
|
||
| import iconSvgHome from './assets/app-home.svg' | ||
| import iconSvgDefault from './assets/app-default.svg' | ||
|
|
||
| const DEFAULT_SIZE = 22 | ||
| const DEFAULT_RADIUS = 5 | ||
|
|
||
| const AppIcon = ({ app, src, size, radius, ...props }) => { | ||
| if (radius === -1) { | ||
| radius = size * (DEFAULT_RADIUS / DEFAULT_SIZE) | ||
| } | ||
| return ( | ||
| <div | ||
| css={` | ||
| display: flex; | ||
| align-items: center; | ||
| overflow: hidden; | ||
| border-radius: ${radius}px; | ||
| `} | ||
| {...props} | ||
| > | ||
| {(() => { | ||
|
There was a problem hiding this comment. Wow! There was a problem hiding this comment. Sure, I don’t have any preference for one or the other (until we have pattern matching 💥), I tend to use this pattern because I find it more readable than using nested ternary operators, and declaring + calling a function at render time is something we do a lot with render props, so I don’t think it is an issue performance wise (as long as we don’t render at 60FPS!). Having internal components is nice too, but the problem is that ESLint insists that every component requires prop types, which makes it super verbose. I couldn’t find any setting to only apply this rule to exported components. Maybe we could directly call functions for this? That would also skip the extra step of creating a React element + having React calling it (insignificant gain in most cases, but it can be an argument to decide). function MyComponent(props) {
return (
<div>
{myComponentContent(props)}
</div>
)
}
function myComponentContent({ a, b }) {
if (a) {
return <span>{a}</span>
}
if (b) {
return <span>{b}</span>
}
return <span>default</span>
}Another solution would be to disable this rule after the main component, which would make it possible to have light internal components: function MyComponent(props) {
return (
<div>
<MyComponentContent {...props} />
</div>
)
}
MyComponent.propTypes = { /* … */ }
// Disabling the ESLint prop-types check for internal components.
/* eslint-disable react/prop-types */
function MyComponentContent({ a, b }) {
if (a) {
return <span>{a}</span>
}
if (b) {
return <span>{b}</span>
}
return <span>default</span>
}And for simple cases, I think the “IIFE in JSX” pattern, while not being the most elegant thing in the world, can do a good job too 😆 function MyComponent({ a, b }) {
return (
<div>
{(() => {
if (a) {
return <span>{a}</span>
}
if (b) {
return <span>{b}</span>
}
return <span>default</span>
})()}
</div>
)
}@AquiGorka @sohkai I don’t really have any preference, I think the three solutions are fine and I use them depending on the context. I pushed EDIT: Adding a few other possibilities for the sake of completeness: Mutable variable (no early returns 😩): function MyComponent({ a, b }) {
let content = <span>default</span>
if (a) {
content = <span>{a}</span>
}
if (b) {
content = <span>{b}</span>
}
return (
<div>
{content}
</div>
)
}Wrapping function: function MyComponent({ a, b }) {
const wrap = content => <div>{content}</div>
if (a) {
return wrap(<span>{a}</span>)
}
if (b) {
return wrap(<span>{b}</span>)
}
return wrap(<span>default</span>)
}Nested ternary: function MyComponent({ a, b }) {
return (
<div>
{a ? (
<span>{a}</span>
) : b ? (
<span>{b}</span>
) : (
<span>default</span>
)}
</div>
)
}There was a problem hiding this comment. ❤️ @bpierre thank you for taking the time to explain your line of thinking and yes I prefer There was a problem hiding this comment. Cool :-) Let’s not make it a convention for now then, and we can refer to this discussion if we want to avoid one of these patterns in the future! |
||
| if (src) { | ||
| return <IconBase size={size} src={src} /> | ||
| } | ||
| if (!app) { | ||
|
There was a problem hiding this comment. Could remove this condition if we flip it to be |
||
| return <IconDefault size={size} /> | ||
| } | ||
| if (app.appId === 'home') { | ||
| return <IconHome size={size} /> | ||
| } | ||
| if (app.baseUrl) { | ||
| const iconUrl = appIconUrl(app) | ||
| return ( | ||
| // Tries to load the app icon while displaying the default one. | ||
| <RemoteImage size={size} src={iconUrl}> | ||
| {({ exists }) => | ||
| exists ? ( | ||
| <IconBase size={size} src={iconUrl} /> | ||
| ) : ( | ||
| <IconDefault size={size} /> | ||
| ) | ||
| } | ||
| </RemoteImage> | ||
| ) | ||
| } | ||
| return <IconDefault size={size} /> | ||
| })()} | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| AppIcon.propTypes = { | ||
| app: PropTypes.object, | ||
| src: PropTypes.string, | ||
| radius: PropTypes.number, | ||
| size: PropTypes.number.isRequired, | ||
| } | ||
|
|
||
| AppIcon.defaultProps = { | ||
| app: null, | ||
| src: null, | ||
| radius: -1, | ||
| size: DEFAULT_SIZE, | ||
| } | ||
|
|
||
| // Base icon | ||
| const IconBase = ({ src, size, alt = '', ...props }) => ( | ||
| <img {...props} src={src} width={size} height={size} alt={alt} /> | ||
| ) | ||
| IconBase.propTypes = { | ||
| alt: PropTypes.string, | ||
| src: PropTypes.string, | ||
| radius: PropTypes.number, | ||
| size: PropTypes.number.isRequired, | ||
| } | ||
|
|
||
| // Default icon | ||
| const IconDefault = props => <IconBase {...props} src={iconSvgDefault} /> | ||
|
|
||
| // Home app icon | ||
| const IconHome = props => <IconBase {...props} src={iconSvgHome} /> | ||
|
|
||
| export default AppIcon | ||
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 just started wondering if I should use
above('medium')instead of!below('medium')