feat(fe): New top section in Personal Projects limits dialog - #4812
feat(fe): New top section in Personal Projects limits dialog#4812michalspeckle wants to merge 2 commits into
Conversation
… into michal/animated-top-section-in-personal-projects-limits-dialog
andrewwallacespeckle
left a comment
andrewwallacespeckle
left a comment
There was a problem hiding this comment.
Nice work! My comments are all nitpicks, but still patterns we try to follow.
Let me know if it's easier to go over some of this on a call, I'm available all day :)
| @@ -0,0 +1,63 @@ | |||
| <template> | |||
| <div | |||
| class="bg-foundation dark:bg-foundation-2 w-full relative border-b border-outline-2 h-full overflow-clip flex justify-center" | |||
There was a problem hiding this comment.
Is there a benefit in using overflow-clip instead of overflow-hidden?
I don't think clip is as well supported, so i'd use overflow-hidden if it works too.
| ></div> | ||
|
|
||
| <div | ||
| class="absolute shadow-2xl rounded-md my-8 p-2 gap-2 flex align-middle top-0 border z-55 w-80 h-full bg-foundation border-outline-2" |
There was a problem hiding this comment.
align-middle uses vertical-align in css, but we normally use flex for this. I'd swap this to items-center
There was a problem hiding this comment.
Also, it's personal preference, and we don't have a set rule for order of tailwind classes, but i try to group related classes, and put positioning ones at the start. This would be absolute z-55 w-80 h-full flex.... in this case
| ></div> | ||
|
|
||
| <div | ||
| class="absolute shadow-2xl rounded-md my-8 p-2 gap-2 flex align-middle top-0 border z-55 w-80 h-full bg-foundation border-outline-2" |
There was a problem hiding this comment.
Is z-55 needed here? can we not use z-10 or z-20?
z-55 would only be used in rare occasions where we want the content to size over all the app. I don't think our dialogs even use such a high number.
| class="bg-foundation dark:bg-foundation-2 w-full relative border-b border-outline-2 h-full overflow-clip flex justify-center" | ||
| > | ||
| <div | ||
| class="z-2 absolute -top-40 left-auto transform rotate-45 rounded-full p-40 border-[150px] border-b-white border-l-indigo-600 dark:border-l-indigo-400 border-r-rose-900 dark:border-r-rose-500 border-t-white blur-[200px]" |
There was a problem hiding this comment.
I think it's ok because this is quite custom and design focussed, but we generally try to avoid redeclaring colours for dark mode. We also try to avoid using newly defined classes like blur-[200px] where possible, but i understand there is no class for this one, more just making you aware.
There was a problem hiding this comment.
Point taken on colors, but yeah I needed this here. Same for blur, anything less than 200px wasn't producing good enough result.
| > | ||
| <WorkspaceAvatar name="My workspace" logo="" /> | ||
| <span | ||
| class="h-[30px] flex place-items-center text-foreground-3 text-body-3xs font-medium" |
There was a problem hiding this comment.
place-items-center seems strange here. Maybe just items-center?
There was a problem hiding this comment.
yeah, mixed up flexbox and grid - will fix that next week
| <div | ||
| class="absolute shadow-2xl rounded-md my-8 p-2 gap-2 flex align-middle top-0 border z-55 w-80 h-full bg-foundation border-outline-2" | ||
| > | ||
| <WorkspaceAvatar name="My workspace" logo="" /> |
There was a problem hiding this comment.
Missing logo here
There was a problem hiding this comment.
Can I use some sort of 'undefined'? It throws an error if I remove the logo attribute completely.
| <ul | ||
| class="relative m-0 list-none h-[204px] w-[302px] my-20 p-0 border border-outline-2 flex bg-foundation-page justify-center rounded-md" | ||
| > | ||
| <li class="absolute z-65 justify-center rounded-md p-2 w-full"> |
There was a problem hiding this comment.
Another high z-index. If we start them all lower we should be able to keep them low.
| </span> | ||
| </div> | ||
| <ul | ||
| class="relative m-0 list-none h-[204px] w-[302px] my-20 p-0 border border-outline-2 flex bg-foundation-page justify-center rounded-md" |
There was a problem hiding this comment.
It feels wrong declaring height and width to the pixel like this, but again, if it's needed for the design to work consistently, i'm happy with it.
| <li class="absolute z-65 justify-center rounded-md p-2 w-full"> | ||
| <div class="flex justify-between w-full gap-2 h-20"> | ||
| <div | ||
| class="absolute h-20 w-[90px] card-slide-in border border-outline-2 bg-foundation rounded-md p-4 place-items-center flex-1" |
There was a problem hiding this comment.
place-items-center and flex-1 seems wrong again, especially when you have defined a set height and width.
Let me know if you want to jump on a call and go over some of the flex alignment classes and how we use them.
There was a problem hiding this comment.
Thanks @andrewwallacespeckle - I was in a rush and <add more excuses>.
Also, my work from this PR is already in Fabians' approved PR, so added a paper cut ticket to improve things next week: https://linear.app/speckle/issue/WEB-3516/refactor-styles-in-intro-dialog-personal-project-limits
I will keep this PR alive for a bit longer.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (70.35%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #4812 +/- ##
==========================================
+ Coverage 70.33% 70.35% +0.01%
==========================================
Files 579 579
Lines 25032 25032
Branches 3733 3733
==========================================
+ Hits 17607 17611 +4
+ Misses 6257 6253 -4
Partials 1168 1168 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
@fabis94 The MovePersonalProjectDialogTopSection.vue should replace the content in the
<div v-if="!condensed"></div>of the LayoutDialog for personal projects limits. The other file is just the SVG I use in the section.Here's a preview of how it should look together with the bottom part:
Description & motivation
Changes:
To-do before merge:
Screenshots:
Validation of changes:
Checklist:
References