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

Implement dashboard layout #13

Merged
merged 27 commits into from
Jul 23, 2024
Merged

Implement dashboard layout #13

merged 27 commits into from
Jul 23, 2024

Conversation

arkadiuszbachorski
Copy link
Collaborator

Implement dashboard layout

♻️ Current situation & Problem

Dashboard needs layout with accessible navigation and profile settings.

⚙️ Release Notes

  • Created Avatar, Tooltip, DropdownMenu, Table, DataTable components
  • Created dashboard layout
  • Added Nil, useOpenState and other utilities

Table and DataTable are pretty basic for now, they'll grow according to our future needs.

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@@ -3,6 +3,13 @@
"parserOptions": {
"project": "./tsconfig.json"
},
"settings": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It allows ESLint plugin to resolve path alias

user: UserInfo
}

export const User = ({ user }: UserProps) => (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Composed out of design system components, but can't be design system itself, because different projects might require different user options

modules/firebase/guards.tsx Show resolved Hide resolved
packages/design-system/.storybook/main.js Show resolved Hide resolved
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 73.75566% with 58 lines in your changes missing coverage. Please review.

Project coverage is 61.47%. Comparing base (6fdf440) to head (024e9f2).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #13       +/-   ##
===========================================
+ Coverage   50.14%   61.47%   +11.34%     
===========================================
  Files          51       73       +22     
  Lines         359      558      +199     
  Branches       51       90       +39     
===========================================
+ Hits          180      343      +163     
- Misses        171      204       +33     
- Partials        8       11        +3     
Files Coverage Δ
app/layout.tsx 0.00% <ø> (ø)
...ges/design-system/src/components/Avatar/Avatar.tsx 100.00% <100.00%> (ø)
...ages/design-system/src/components/Avatar/index.tsx 100.00% <100.00%> (ø)
...system/src/components/DataTable/DataTable.mocks.ts 100.00% <100.00%> (ø)
...s/design-system/src/components/DataTable/index.tsx 100.00% <100.00%> (ø)
...esign-system/src/components/DropdownMenu/index.tsx 100.00% <100.00%> (ø)
...kages/design-system/src/components/Table/index.tsx 100.00% <100.00%> (ø)
...s/design-system/src/components/Tooltip/Tooltip.tsx 100.00% <100.00%> (ø)
...ges/design-system/src/components/Tooltip/index.tsx 100.00% <100.00%> (ø)
.../src/molecules/DashboardLayout/DashboardLayout.tsx 100.00% <100.00%> (ø)
... and 20 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fdf440...024e9f2. Read the comment docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Comment on lines +54 to +56
<span className="interactive-opacity w-full px-2 pt-4 text-center">
Logo
</span>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Logos might look differently depending on the logo, so leaving it up to user-land

<span className="interactive-opacity w-full px-2 pt-4 text-center">
Logo
</span>
<nav className="mt-24 flex flex-col gap-1 xl:w-full">{menuLinks}</nav>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same here, some apps might have nested menus and stuff, leaving it up to user-land

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for all the work here @arkadiuszbachorski; I only had a few smaller layout suggestions but those might be easy to address. The individual components look great, thanks for getting them into place and adapting them to our project.

const user = <User user={getUserInfo(currentUser)} />

return (
<DashboardLayoutBase
Copy link
Member

Choose a reason for hiding this comment

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

That is very cool. Thank you for the outline.

I have a few high-level feedbacks on the UI components:

  1. Would be good to move this closer to the top, maybe below the horizontal title line with some small padding?
  2. I have the feeling this main bar has a bit too much vertical (and maybe slightly horizontal) spacing, might be cool to explore how this can be reduced a bit, e.g. taking the GitHub Main Bar as some inspiration.
  3. I would suggest making the sidebar a bit wider in its expanded form. It might be a bit of a bias, but it would be great if my name would fit without truncation 😄
Screenshot 2024-07-15 at 4 45 06 PM

I have also encountered a small bug:

  1. Resize the browser to go into the mobile view
  2. Open the menu in the view by pressing the hamburger menu
  3. Resize the view again to its full size: The menu stays open, but the button to dismiss it disappears. The expected behavior would be to show the normal navigation menu size instead; there might be some state handling here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Would be good to move this closer to the top, maybe below the horizontal title line with some small padding?

Sure, that works too.

Decrease vertical space

  1. I have the feeling this main bar has a bit too much vertical (and maybe slightly horizontal) spacing, might be cool to explore how this can be reduced a bit, e.g. taking the GitHub Main Bar as some inspiration.

Header bar is just mostly empty for now. It can contain way more, like action button, search input, controls, tabs, breadcrumbs. I think your feeling might be caused because of it's current emptiness. When you compare it against GitHub Main bar, it's way more crowded, but at the same GH's bar is even more vertical (86px ours vs 106px GH). I'd keep as is for now and possibly iterate later if needed.

  1. I would suggest making the sidebar a bit wider in its expanded form. It might be a bit of a bias, but it would be great if my name would fit without truncation 😄

Mine doesn't fit either 😄 It's not easy to fit our names! I would love not to expand sidebar more, because it's going to occupy more working space.

I played around making dashboard more compact in general. Let me know what you think
Make Dashboard more compact

I was very generous with white space, but more compact design is more universal.

I have also encountered a small bug

Fixed, thanks for catching it. It was actually caused by design system vs app classes clash. I might resolve it different way in the future, but for now this is ok. Fix classes clash

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for looking at all of this! Will check this out later today but feel free to merge the PR if you feel comfortable in merging it before 👍

modules/firebase/guards.tsx Show resolved Hide resolved
packages/design-system/src/components/Avatar/Avatar.tsx Outdated Show resolved Hide resolved
@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Jul 16, 2024
Base automatically changed from arek/add-firebase to main July 23, 2024 18:27
@arkadiuszbachorski arkadiuszbachorski merged commit ceaa0ba into main Jul 23, 2024
10 checks passed
@arkadiuszbachorski arkadiuszbachorski deleted the arek/add-dashboard branch July 23, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants