Use the same component to render every app icon

[bpierre]

Use a single component for app icons: AppIcon.

• This component replaces RemoteIcon and Permissions/AppIcon.
• Manages displays the app icon or the default, based on the appId.
• Accepts an src parameter too (useful for the Apps screen).
• The radius is automatic but can also be set independently from the size.

Also fixes the home icon alignment on Chrome.

[sohkai]

Maybe we should place this in the icons directory?

[bpierre]

I went a bit farther and refreshed the way we manage app icons in general: edfc2cf

[AquiGorka]

I just started wondering if I should use above('medium') instead of !below('medium')

[AquiGorka]

Wow!
Would it have been cleaner to use another component instead of a self executing function?

[bpierre]

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 #2 for now as @AquiGorka was suggesting a preference for it rather than #3. Let me know if you have any opinion about it so we align!

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>
  )
}

[AquiGorka]

❤️ @bpierre thank you for taking the time to explain your line of thinking and yes I prefer #2 but it is only a personal preference and I can adhere to any team convention we agree on - and yes, because of prop types it becomes a bit too verbose.

[bpierre]

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!

[AquiGorka]

❤️ Thank you! For a minute I thought I introduced the bug in chrome...

[sohkai]

LGTM!

[sohkai]

Could remove this condition if we flip it to be if (app)