[dashboard] Fix Teams & Projects top-level menu UX #5077
Conversation
There was a problem hiding this comment.
@jankeromnes this looks certainly much better than before!
UX looks good to merge. Feel free to request a review for the frontend code if needed. 🏀
Since we're here what do you think of also changing the hover color of the dropdown entries to bg-gray-100? 🤓
Optionally, if this takes less only a few minutes to fix we could also highlight the actively selected team as described in #5072. However, feel free to leave this out of the scope of these changes. ✂️
|
LGTM label has been added. Git tree hash: cecce1b0a8049d3c9f9c9099e00829aa8d79b33d
|
|
Many thanks for the quick review! ⚡ Will take a look at the proposed improvements. 😁 /approve /hold |
@gtsiolis Is it cool to change them for all |
jankeromnes
force-pushed
the
jx/teams-projects-polish
branch
from
2469fbb to
0c3ae1a
Compare
@jankeromnes Yeap! This is a general improvement that can benefit all other dropdowns as the component design evolves over time. 🚢 |
Awesome! Done ✅ 🎉
Hmm, this isn't super easy, since the team selector uses the However, an easy fix could be to make the
(I could also quickly fix the background color "cutting into" the corners.) |
jankeromnes
force-pushed
the
jx/teams-projects-polish
branch
2 times, most recently
from
00f8b89 to
e59103c
Compare
|
Okay, I think the "active"
In dark, we use the same
@gtsiolis Still looks good to you? |
There was a problem hiding this comment.
Looks great @jankeromnes!
Left two minor comments. Besides these, UX looks good to merge! 🚢
The changes here would probably also resolve #5073, right?
| @@ -84,7 +84,7 @@ function ContextMenu(props: ContextMenuProps) { | |||
| <div className={`mt-2 z-50 bg-white dark:bg-gray-900 absolute flex flex-col border border-gray-200 dark:border-gray-800 rounded-lg truncated ${props.classes || 'w-48 right-0'}`}> | |||
| {props.menuEntries.map((e, index) => { | |||
| const clickable = e.href || e.onClick || e.link; | |||
| const entry = <div className={`px-4 flex py-3 ${clickable ? 'hover:bg-gray-200 dark:hover:bg-gray-800' : ''} text-sm leading-1 ${e.customFontStyle || font} ${e.separator ? ' border-b border-gray-200 dark:border-gray-800' : ''}`} title={e.title}> | |||
| const entry = <div className={`px-4 flex py-3 ${clickable ? 'hover:bg-gray-100 dark:hover:bg-gray-800' : ''} ${e.active ? 'bg-gray-50 dark:bg-gray-800' : ''} ${index === 0 ? 'rounded-t-lg' : ''} ${index === props.menuEntries.length - 1 ? 'rounded-b-lg' : ''} text-sm leading-1 ${e.customFontStyle || font} ${e.separator ? ' border-b border-gray-200 dark:border-gray-800' : ''}`} title={e.title}> | |||
There was a problem hiding this comment.
issue: This is minor but there's a small gap on the corners due to corner radiuses. It should be barely noticable but if there's an easy fix let's also include it here. Otherwise, a follow up issue could suffice. 🏓
| A | B |
|---|---|
 |
 |
There was a problem hiding this comment.
Oh no 🙈 that's so unsatisfying 😬
There was a problem hiding this comment.
Follow-up issue: #5086
|
LGTM label has been added. Git tree hash: 6c62fdd49576eb88680dfbc0fe8d7860f3a92c6c
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gtsiolis, jankeromnes Associated issue: #4622 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jankeromnes
force-pushed
the
jx/teams-projects-polish
branch
from
e59103c to
82564e2
Compare
|
New changes are detected. LGTM label has been removed. |
|
Okay, should be good to launch as is! 🚀 Releasing the breaks: /unhold |
Fixes #4622
@gtsiolis What do you think about this change? Is it a good trade-off? (Nicer on root pages, but still allows breadcrumb navigation when there are breadcrumbs.)
EDIT: Now also fixes #5072 and fixes #5073