Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Looks pretty neat!
It seems to be missing the app titles on the top though. |
|
Also, every time I open an app, it shows the legal disclaimer for a split second. |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
| { selectedApp, safeAddress, network, appIsLoading, granted, onIframeLoad }, | ||
| iframeRef, | ||
| ): React.ReactElement { | ||
| const AppFrame = ({ appUrl }: Props): React.ReactElement => { |
There was a problem hiding this comment.
AppFrame was meant to be a component that is responsible for displaying the app inside an iframe and not hold any logic. Why did you move the logic here?
There was a problem hiding this comment.
Before this change, separating the logic made sense, mixing the app list logic with the app frame was chaos. Now App frame is rendered in a new URL and the logic here is merely to determine what to render.
There was a problem hiding this comment.
mixing the app list logic with the app frame
app list logic was hidden inside useAppList hook, wasn't it?
There was a problem hiding this comment.
yes, but we don't want to make use of useAppList in AppFrame, it will load the complete app list for no reason.
That's why I moved to AppFrame the removeApp functionality.
I can create another hook for that functionality if you think, we can also move the logic that checks if an app.url exits in the static list
| const handleSubmit = () => { | ||
| closeModal() | ||
| onAppAdded(appInfo) | ||
| const newAppList = [ | ||
| { url: appInfo.url, disabled: false }, | ||
| ...appList.map(({ url, disabled }) => ({ url, disabled })), | ||
| ] | ||
| saveToStorage(APPS_STORAGE_KEY, newAppList) | ||
| const goToApp = `${matchSafeWithAddress?.url}/apps?appUrl=${encodeURI(appInfo.url)}` | ||
| history.push(goToApp) | ||
| } |
There was a problem hiding this comment.
This only updates the stored app list and not app's state, is it intended?
There was a problem hiding this comment.
sorry, what do you mean by app's state?
There was a problem hiding this comment.
we hold an apps list in state inside useAppList hook
There was a problem hiding this comment.
ahh, I see what you mean. No, it's not needed anymore, because the useAppList is only used in /apps URL, when we add a new app, we redirect it to /apps/new_app_url.
then, if the user goes back to /apps it will load the just added_app because it's already in the storage.
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
I've checked the list in Chrome, Firefox and Brave and the list looks fine. I've check the correction of the issues reported in previous comments: Looks good to me |
7 participants
Closes #1294 #1282.
It's there some kind of delay until all the apps finished the loading. I'll be taking a look tomorrow.All the functionality should be implemented.