ipfs-shipyard / ipfs-share-files

Share files via IPFS
https://share.ipfs.io
MIT License
149 stars 31 forks source link

fix: translated box #31

Closed fsdiogo closed 6 years ago

fsdiogo commented 6 years ago

This fixes the breaking BoxNotAvailable because I wasn't importing the right one in the Page.js.

fsdiogo commented 6 years ago

@olizilla I'm not 100% happy with this solution: having the rawX and then exporting the "full" component.

Do you think there is a better way or am I just being picky?

olizilla commented 6 years ago

It is mostly just a style question. The codebase don't mind what you call them.

I like the convention of the most simple presentation component, with zero wrapping, getting the simplest name, so

The translation wrapping is an interesting case. It's totally fine and desirable to pass in mock values for properties and actions that would come from the redux store, or any other wrapping HOC, as we just want to see the presentation in the storybook. As the text is a core part of the presentation and lots of UI issues hide in fixed width things breaking when translated, we do want to export the component with the translation wrapper, as passing in dummy values is unhelpful as it hides those bugs.

So we could simplify things and have the Box component export itself with the translate wrapper. We'd saying that you can only use this component in an environment that has an i18nProvider higher up the tree, which isn't unreasonable as the component has some text baked in.

export const Box = translate()(({t}) => {
  return ( 
    <div>{t('foo')}</div>
 )
})

is ok, but looks a little ugly.

export const Box = () => {
  <I18n>
    (t) => (
          <div>{t('foo')}</div>
    )      
}

is ok, but the I18n render props component is a little verbose. Either is fine. Both says "Hey, I depend on an I18nProvider.

olizilla commented 6 years ago

The <I18n> component with t as a render prop / children as fn style is documented here https://react.i18next.com/components/i18n-render-prop#sample-usage

olizilla commented 6 years ago

I think what you have in this PR is fine. I'd rename it RawBox, with a capital R so it's a legit, jsx ready component. If you want to collapse RawBox and Box into 1 thing, then that is fine too.

fsdiogo commented 6 years ago

Great insight Oli, thank you 👌

I'll keep it as is but change the name to a capital first letter then! 🌟