feat: Open modals with route instead pushModal #2978
Conversation
BundleMonFiles updated (5)
Unchanged files (14)
Total files change -3.87KB -0.09% Groups updated (3)
Unchanged groups (5)
Final result: ✅ View report in BundleMon website ➡️ |
| .fil-mobileactionmenu-header | ||
| font-weight bold | ||
| max-width 100% | ||
| display flex |
| <Icon icon={ShareIcon} /> | ||
| </ListItemIcon> | ||
| <ListItemText primary={t('Files.share.cta')} /> | ||
| {isMobile && props.doc ? ( |
There was a problem hiding this comment.
why did you add props.doc?
There was a problem hiding this comment.
Previously, we obtained a files property but only used the first element. With MenuActions, only the document linked to the action is shared with the doc property. I use it to get the identifier to display the recipients
| ) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
I didn't understand why you have to check isArray() everywhere and then [files]. Can you explain?
There was a problem hiding this comment.
Your comment makes me think that I haven't documented this point.
Actions are used in two places in a file's drop-down menu and in the selection bar. Before the migration to ActionsMenu, we always received an array in both case. The ActionsMenu now sends the document directly to the action, while the SelectionBar sends an array of documents. This code seeks to manage the two cases.
I'm going to add a comment in the commit for the future me
There was a problem hiding this comment.
Nice work.
A remark for later since I don't have a strong opinion on it yet:
You added a route for every possible action on every level. Now there is a lof of declared routes. And each time we'll add an action, we'll have to add 3 routes. I love the "declarative" approach, but I don't know if it's suitable.
Maybe we can create 3 routes :
/files/:id/action/:action
And then, the action component linked to these route will do the job to check the action param to display the right modal. In that case, if we had an action, we'll not have to add new routes, but just editing this action component
|
This would really make sense to simplify the routes and share the file request logic. We could do something similar with another route for actions related to multiple files, such as moving or deleting. I'll ask myself this question again when I migrate the last remaining modals using pushModal |
Actions are interpreted in two places, in ActionsMenu and in SelectionBar. Before migration, action function always received an array of documents in both case. After migration, ActionsMenu now sends the document directly, while the SelectionBar sends an array of documents. To manage the two cases, I used an verification on if the params was an Array or not. In the case, we expected a single file if its an array I take the first element otherwise I put the single file into an array.
cballevre
force-pushed
the
feat/modal-as-route
branch
3 times, most recently
from
c8d1eb2 to
2ae0aeb
Compare
The purpose of this is to reuse it in other components that are not linked to the move functionality
cballevre
force-pushed
the
feat/modal-as-route
branch
from
2ae0aeb to
1533bbc
Compare
This aims to make easier to navigate into the Flagship application