-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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