-
Notifications
You must be signed in to change notification settings - Fork 6
feat(ui): create separate pages for item share, publish and settings #971
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! Removing all these tabs was really a good idea and removed bloating code, don't you think? Be careful of duplication, I think it can be easily avoided ;) Let me know if you have any questions.
Also UI reviews:
- Should we have the back button outlined? I think we don't see it enough. (we discuss it on slack)
- Clear chat button has a strange look now, was it all centered before?
5938860
to
ccf769a
Compare
<UppyContextProvider enable={enableEditing} itemId={itemId}> | ||
<Box py={1} px={2}> | ||
<Box display="flex" alignItems="center"> | ||
<BackButton onClick={() => navigate(buildItemPath(itemId))} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this create a "forward" navigation ? Maybe we could "rewind" the navigation ? But maybe that is a bit more involved ?
export const buildItemSettingsPath = (id = ':itemId'): string => | ||
`${ITEMS_PATH}/${id}/${ITEM_SETTINGS_PATH}`; | ||
export const buildItemSharePath = (id = ':itemId'): string => | ||
`${ITEMS_PATH}/${id}/${ITEM_SHARE_PATH}`; | ||
export const buildItemPublishPath = (id = ':itemId'): string => | ||
`${ITEMS_PATH}/${id}/${ITEM_PUBLISH_PATH}`; | ||
export const buildItemInformationPath = (id = ':itemId'): string => | ||
`${ITEMS_PATH}/${id}/${ITEM_INFORMATION_PATH}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think react-router-dom has some methods to resolve these paths depending on where they are created. look at useResolvedPath
which allows to use a relative path that is then resolved to the absolute path depending where you are in the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice !
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
closes Create page for share, publish, informations and settings #955