Skip to content
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

storage navigation refactor #2196

Merged
merged 11 commits into from
Jun 23, 2022
Merged

storage navigation refactor #2196

merged 11 commits into from
Jun 23, 2022

Conversation

tanmoyAtb
Copy link
Contributor

closes #2091

We now have Api Keys and Billing nav items in place.
Settings are kept hidden.

@render
Copy link

render bot commented Jun 20, 2022

@render
Copy link

render bot commented Jun 20, 2022

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Jun 20, 2022
@render
Copy link

render bot commented Jun 20, 2022

@tanmoyAtb
Copy link
Contributor Author

The settings tests are failing. Going to add the updates.

@asnaith
Copy link
Contributor

asnaith commented Jun 20, 2022

Working well and looks good for the most part but I'm a little bit undecided on whether this spacing makes sense now:

Screen Shot 2022-06-20 at 3 15 03 PM

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

The code looks good. I have no strong opinion regarding the spacing, I'm undecided.

Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

Same as Tbaut, I'm not so sure about the spacing, maybe @serenaho can help?

Regarding the changes, looks good! waiting for the fix of the tests to approve it

packages/storage-ui/src/Components/Layouts/AppNav.tsx Outdated Show resolved Hide resolved
@serenaho
Copy link

Included some options here on Figma!

Options A, B, and C are pretty straightforward but options D and E have Docs and Billing separated as buttons because they don't quite fit under the category as a menu item like the others.

Happy to discuss this during the planning meeting tmr :)

@Tbaut
Copy link
Collaborator

Tbaut commented Jun 22, 2022

This could be for another ticket but:

  • I think the docs being an external link should indeed be separate
  • I think we prob don't need headers or lines, there's already enough text
  • Billing is a wrong name, I think we use "Subscription" in Files

My suggestion would be all together but Docs at the bottom of the nav bar

Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

Subscription test were failing in Files, but was a problem with the API that was fixed, I rerun the tests and now they all passed.

Everything fine now! I guess we leave the spacing improvement for another ticket

@tanmoyAtb tanmoyAtb merged commit 7515ccf into dev Jun 23, 2022
@tanmoyAtb tanmoyAtb deleted the feat/storage-navigation-2091 branch June 23, 2022 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Storage] Main Navigation Refactor
7 participants