Ask permissions for apps if the list of apps can't be fetched #155
Conversation
src/components/AppsList.jsx
Outdated
| <hr /> | ||
| </div> | ||
| ) | ||
| })} |
There was a problem hiding this comment.
The whole return is too big, we shall divide it into smaller functionnal structure. Maybe create a functionnal component for the map and for the ternary operator.
As this it consumes way too much of my neurons.
There was a problem hiding this comment.
That's why I extracted the AppIconand AppIconGroup: it's not only for the FakeAppList, this is still a WIP
There was a problem hiding this comment.
Still too big : 40 lines and 3 return statements.
There was a problem hiding this comment.
Yes, I know, I will do it, that's why I said it's WIP :)
| <AppIcon label='Cozy Photos' iconSrc={require('../assets/icons/apps/icon-photos.svg')} /> | ||
| <AppIcon label='Cozy Collect' iconSrc={require('../assets/icons/apps/icon-collect.svg')} /> | ||
| <AppIcon label='Cozy Store' comingSoon iconSrc={require('../assets/icons/apps/icon-store.svg')} /> | ||
| </AppIconGroup> |
There was a problem hiding this comment.
We shall refactor this with a Map
const apps = {
'Cozy Drive': '../assets/icons/apps/icon-drive.svg',
'Cozy Photos': '../assets/icons/apps/icon-photos.svg',
'Cozy Collect': '../assets/icons/apps/icon-collect.svg',
'Cozy Store': '../assets/icons/apps/icon-store.svg'
}and a map
Object.keys(apps).map(key => (
<AppIcon label={key} iconSrc={apps[key]} />
))There was a problem hiding this comment.
Not sure we gain much by doing this... and Cozy store needs its comingSoon argument
There was a problem hiding this comment.
You could add a comingSoon statement, I just gave an example to be inspired by.
We gain much as it's more DRY.
| @@ -30,7 +30,7 @@ class Drawer extends Component { | |||
| } | |||
|
|
|||
| render () { | |||
| const { onClaudy, visible, isClaudyLoading, toggleSupport } = this.props | |||
| const { onClaudy, visible, isClaudyLoading, toggleSupport, renewToken } = this.props | |||
There was a problem hiding this comment.
This looks to me very weird for a Drawer component to need a renewToken prop.
There was a problem hiding this comment.
Yep, but I don't see a better way of passing this function to the FakeAppList
There was a problem hiding this comment.
I did not take this any further, but it's very weird.
src/components/SupportModal.jsx
Outdated
| @@ -12,6 +12,7 @@ class SupportModal extends Component { | |||
| } | |||
|
|
|||
| toggle = () => { | |||
| console.log('toggle') | |||
| dispatch(receiveAppListForbidden()) | ||
| } else { | ||
| console.warn(e.message ? e.message : e) | ||
| } |
There was a problem hiding this comment.
We shall wrap by a try/catch the only instruction that need it. Maybe by extracting this instruction:
Instead of
try {
cont app = getApp()
app.doSomething()
} catch (Exception e) {
dispatch(noApp())
}we can do
const getAppWithExceptionHandling = () => {
try {
return getApp()
} catch (Exception e) {
return null
}
}
const app = getAppWithExceptionHandling()
if(app === null) {
dispatch(noApp())
return
}
app.doSomething()What do you think?
There was a problem hiding this comment.
The stack.get.apps() call is not the only instruction needing to be try/catched, there are also the stack.get.icon() calls, that's why I put everything in the try/catch (and besides that, I need to dispatch a very specific action for a very specific error, so your suggestion wouldn't work). I thought splitting this code even more could make it less readable, and I already made it more clearer than it was imho.
fix: From review fix: responsive permission modal feat: Handled the renewed token fix: Forgotten appName in translated desc feat: Added className so that we can hide some buttons on mobile
goldoraf
changed the title
|
LGTM 👍 |
Until now we didn't show the cozy-bar on Drive mobile, but we want to enable some parts of it on mobile right now. The pb is that some users that installed previous versions of Drive mobile didn't request the
appspermissions when connecting their device to their Cozy, and are consequently unable to display the bar's app list. This PR adds a modal that asks the user for these permissions and renews the token so that the apps can be fetched. The modal's background is a fake app list with blurry icons, that's why I had to add the SVGs.Imgur