Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Use subtabs in sidebar #2521

Merged
merged 15 commits into from
Jul 13, 2021
Merged

Use subtabs in sidebar #2521

merged 15 commits into from
Jul 13, 2021

Conversation

dasanra
Copy link
Collaborator

@dasanra dasanra commented Jul 8, 2021

What it solves

Resolves #1295

How this PR fixes it

This PR adds subtabs menu for ASSETS and SETTINGS, making easier to discover collectibles and making the view for the settings bigger

How to test it

Just load a safe and navigate using the sidebar menu. Check for the expected behavior when clicking around

Screenshots

Notification badge example for current PR
image

@dasanra dasanra self-assigned this Jul 8, 2021
@github-actions
Copy link

github-actions bot commented Jul 8, 2021

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Jul 8, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@github-actions
Copy link

github-actions bot commented Jul 8, 2021

@github-actions
Copy link

github-actions bot commented Jul 8, 2021

@github-actions
Copy link

github-actions bot commented Jul 8, 2021

@github-actions
Copy link

github-actions bot commented Jul 8, 2021

@dasanra dasanra marked this pull request as draft July 8, 2021 16:29
@lukasschor
Copy link
Member

Looks really good! Especially the settings. 🎉

I realize the PR is still in draft, but wanted to leave some things I saw so I won't forget about them. Feel free to ignore if these are anyways planned to be addressed.

There is a missing whitespace in Spending Limit
CleanShot 2021-07-08 at 18 21 04@2x

This switch now became redundant. Should we remove it?
CleanShot 2021-07-08 at 18 26 35@2x

Also, while not related to this PR I saw that in the Safe Details section is the only place where we:

  • Use a bigger font for the description
  • Have a line-break in the description

CleanShot 2021-07-08 at 18 27 39@2x

CleanShot 2021-07-08 at 18 27 50@2x

Maybe worth aligning at some point. I personally feel (1) big text and (2) no line-break would leverage the increased screen estate the best and be easiest to read.

@dasanra
Copy link
Collaborator Author

dasanra commented Jul 9, 2021

@lukasschor thank you for your suggestions! I was still working on the old link change, as it had some deeper changes 😉 but all the others were useful!

Once the build is ready all those will be done.

I still have to add, if is still requested the "upgrade" badge that we had (red dot in Safe Details button when a smart contract upgrade is available)

Or maybe we can remove it from scope and create a new ticket in order to add some more standard bubble that can be used for a more generic notification system for any menu entry WDYT?

@dasanra dasanra marked this pull request as ready for review July 12, 2021 10:33
@dasanra dasanra requested a review from katspaugh July 12, 2021 10:33
badge: needsUpdate && granted,
icon: <ListIcon type="info" />,
selected: safeAction === 'settings' && safeSubaction === 'details',
href: `${matchSafeWithAddress?.url}/settings/details`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bit of duplication in this config. I would suggest defining a more static config and build this one from it.

Like so:

const makeItem = (config) => ({
  label: config.label,
  badge: needsUpdate && granted,
  icon: <ListIcon type={config.id} />,
  selected: safeAction === config.sectionId && safeSubaction === config.id,
  href: `${matchSafeWithAddress?.url}/${config.sectionId}/${config.id}`,
})

makeItem({
  label: ''Safe Details',
  id: 'details',
  sectionId: 'settings'
}),
makeItem({
  ...
}),

Copy link
Collaborator Author

@dasanra dasanra Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this change has a big impact. Will only save us from writting <ListIcon /> and matchSafeWithAddress?.url/. config.id abstraction done here is not correct, Icon type doesn't always match with subSection name. Worst case we will always need the same number of properties to write this:

makeItem({
  label: 'Safe Details',
  badge: (true or false, needs to be parametrized for each case)
  icon: 'settingsTool',
  sectionId: 'settings',
  subSectionId: 'subSection'
})

src/components/List/index.tsx Outdated Show resolved Hide resolved
src/components/List/index.tsx Outdated Show resolved Hide resolved
src/components/List/index.tsx Outdated Show resolved Hide resolved
render={() => <SpendingLimitSettings />}
></Route>
<Route
path={`${SAFELIST_ADDRESS}/${address}/settings/advanced`}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These URLs are duplicated here and in the menu. Ideally they should come from the same enum, and the menu should use generatePath.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With current implementation I can only do a partial refactor for this suggestion. I won't be able to use generatePath, as SAFELIST_ADDRESS include a / character and generatePath escape those values.

To improve this we should either go for a tech debt ticket because it won't make sense to change it only here or to define two SAFELIST_ADDRESS values in src/routes/routes meanwhile we can't do the full migration

@dasanra dasanra requested a review from katspaugh July 12, 2021 15:16
@lukasschor
Copy link
Member

Screen.Recording.2021-07-12.at.17.30.00.mov

This bouncing behavior seems weird. It happens when you click a category that is already selected.

@francovenica
Copy link
Contributor

Minor issue:
The "back" button in the browser is causing some inconsistencies:
1 - Go to assets tab > Balances
2 - Click in settings (it will go to Settings > safe details by default)
3 - Use the back button

The safe will go back to balances, but the setting menu remains open. The "Assets" tab gets selected, but it doesn't open to show it is in the "Coin" tab
07-12-2021_x(2727)

Same happens if you are in the Settings tab > Safe details, then go to assets > Coin lastly you use the "Back" button of the browser. It will go back to Settings > Safe details, but is the Assets dropdown the one that remains open.

@francovenica
Copy link
Contributor

The bouncer behavior and the issue with the "back" button in browsers were solved.

It looks good to me

@liliya-soroka
Copy link
Member

liliya-soroka commented Jul 13, 2021

@dasanra , is the information about the opened tab is missed or skipped ?
Example: assets/coins
image

@dasanra dasanra merged commit 7e39841 into dev Jul 13, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2021
@katspaugh katspaugh deleted the feature/sub-tabs-in-sidebar branch July 13, 2021 13:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move sub-tabs to sidebar
5 participants