Delegate selector drawer review #38
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
avocawo
force-pushed
the
delegate-selector-drawer-review
branch
from
7b28c91 to
d70da34
Compare
3em
reviewed
Apr 30, 2025
| <Table.Root | ||
| className={isLoading ? '[mask-image:linear-gradient(to_bottom,black_30%,transparent_100%)]' : ''} | ||
| className={cn( | ||
| isLoading ? '[mask-image:linear-gradient(to_bottom,black_30%,transparent_100%)]' : '', |
Collaborator
There was a problem hiding this comment.
We can remove this entire line because, if isLoading is true, we're already rendering SkeletonList above
3em
reviewed
Apr 30, 2025
| className={isLoading ? '[mask-image:linear-gradient(to_bottom,black_30%,transparent_100%)]' : ''} | ||
| className={cn( | ||
| isLoading ? '[mask-image:linear-gradient(to_bottom,black_30%,transparent_100%)]' : '', | ||
| 'overflow-visible', |
3em
reviewed
Apr 30, 2025
| className={cn( | ||
| isLoading ? '[mask-image:linear-gradient(to_bottom,black_30%,transparent_100%)]' : '', | ||
| 'overflow-visible', | ||
| 'pt-[0.3125rem]' |
3em
reviewed
Apr 30, 2025
| <Table.Header> | ||
| <Table.Row> | ||
| <Table.Head className="w-96">Delegate</Table.Head> | ||
| <Table.Head className="w-60">Delegate</Table.Head> |
Collaborator
There was a problem hiding this comment.
All widths should be set using % because this table can potentially be rendered in full-screen mode at any time.
3em
reviewed
Apr 30, 2025
| <Table.Head className="w-44 text-center">Selected</Table.Head> | ||
| </Table.Row> | ||
| </Table.Header> | ||
| {isLoading ? ( |
Collaborator
There was a problem hiding this comment.
We can remove this conditional part of the code because, if isLoading is true, we're already rendering SkeletonList above.
3em
reviewed
Apr 30, 2025
| return ( | ||
| <Table.Row key={groupId}> | ||
| <Table.Cell className="max-w-80 content-center truncate"> | ||
| <Table.Row key={groupId} className="hover:bg-cn-borders-2"> |
Collaborator
There was a problem hiding this comment.
We don’t need this class — we already have the hasHighlightOnHover prop set on Table.Body
3em
reviewed
Apr 30, 2025
| <Table.Row key={groupId}> | ||
| <Table.Cell className="max-w-80 content-center truncate"> | ||
| <Table.Row key={groupId} className="hover:bg-cn-borders-2"> | ||
| <Table.Cell className="max-w-40 content-center truncate"> |
Collaborator
There was a problem hiding this comment.
We don’t need max-width on the body cells because the widths are already defined in Table.Head
3em
reviewed
Apr 30, 2025
| <Table.Cell className="relative min-w-6 text-right"> | ||
| {isDelegateSelected( | ||
| [...defaultTo(groupImplicitSelectors, []), ...defaultTo(groupCustomSelectors, [])], | ||
| selectedTags || [] |
Collaborator
There was a problem hiding this comment.
Let’s set a default value of [] for selectedTags prop and remove this condition here
3em
reviewed
Apr 30, 2025
| selectedTags || [] | ||
| ) && <Icon name="tick" size={12} className="text-icons-success" />} | ||
| ) && ( | ||
| <div className="absolute inset-0 flex w-full items-center justify-center"> |
Collaborator
There was a problem hiding this comment.
I'm not sure you should be using absolute positioning here — it feels overly complex just to display an icon
3em
reviewed
Apr 30, 2025
|
|
||
| return ( | ||
| <SandboxLayout.Content className="h-full px-0 pt-0"> | ||
| <SandboxLayout.Content className="h-[calc(100%-theme('spacing.28'))] py-0"> |
Collaborator
There was a problem hiding this comment.
We should avoid using custom styles here — it looks like we don’t actually need them anyway
3em
reviewed
Apr 30, 2025
| )} | ||
|
|
||
| <div className="absolute inset-x-0 bottom-0 bg-cn-background-2 p-4 shadow-md"> | ||
| <div className="bg-cn-background-2 b-0 sticky inset-x-0 bottom-0 -ml-5 mt-auto w-[calc(100%+theme('spacing.10'))] border-t p-4 shadow-md"> |
Collaborator
There was a problem hiding this comment.
We can leave this part as absolute
3em
reviewed
Apr 30, 2025
| <div className="flex p-5 pb-0"> | ||
| Haven't installed a delegate yet? | ||
| <StyledLink className="ml-1 flex flex-row items-center" variant="accent" to="#"> | ||
| Install delegate <Icon name="attachment-link" className="ml-1" size={12} /> |
Collaborator
There was a problem hiding this comment.
You can add a class with gap-x-1 to StyledLink and remove the ml-1 from the Icon
avocawo
force-pushed
the
delegate-selector-drawer-review
branch
from
bb5a23a to
16535a8
Compare
Color table items on hover. Fixes scroll behavior for empty data list. Changes data table to take all available space. Sticky header and footer segments. Moved delegate-selector and delegate-connectivity under ui package folder. Reuse them in apps/subjects/views.
avocawo
force-pushed
the
delegate-selector-drawer-review
branch
from
fcd4b63 to
2748776
Compare
avocawo
force-pushed
the
delegate-selector-drawer-review
branch
from
137a944 to
ba17869
Compare
avocawo
force-pushed
the
delegate-selector-drawer-review
branch
from
ba17869 to
884ff54
Compare
- Adds ScrollArea element to enable custom scroll. - Fixes UI to match design screens. - Passes RadioButton to control. - Fixes gaps between sections. - Fixes rows height shift on icon appearance. - Fixes radio select component according to design. - Refactors Option and RadioSelect components. - Reverts previous components movements. Moved Drawer to packages/ui only.
avocawo
force-pushed
the
delegate-selector-drawer-review
branch
from
ec45fea to
0c2c7b8
Compare
3em
reviewed
May 2, 2025
| import mockDelegatesList from '@subjects/views/delegates/mock-delegates-list.json' | ||
| import { defaultTo } from 'lodash-es' | ||
|
|
||
| export const useDelegateData = () => |
3em
reviewed
May 2, 2025
| export const Option: FC<OptionProps> = ({ control, id, label, description, ariaSelected, className }) => { | ||
| const hasLabel = !!label | ||
| const hasDescription = !!description | ||
| const hasLabelOrDescription = hasLabel || hasDescription |
Collaborator
There was a problem hiding this comment.
No need to add these constants — the same conditions below would’ve been enough
- Resolves console errors. - Fixes Install delegate link margins. - Fixes select arrow component alignment. - Makes data table fixed. - Adds button hover. - Fixes RadioSelect component styles when content is really big.
avocawo
force-pushed
the
delegate-selector-drawer-review
branch
from
8719aee to
21f0210
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
apps/design-system/src/subjects/views/delegatestopackages/ui/src/views/delegates.