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

fix(admin-ui): various UI fixes #4492

Merged
merged 30 commits into from
Jan 22, 2025
Merged

fix(admin-ui): various UI fixes #4492

merged 30 commits into from
Jan 22, 2025

Conversation

leopuleo
Copy link
Contributor

@leopuleo leopuleo commented Jan 15, 2025

Changes

With this PR introduces multiple changes:

  • Add xs icon size.
  • Fix the form components spacing.
  • Fix the form components ghost variant when disabled: true.
  • Cast Tooltip to font-weight: normal.
  • Improve extendTailwindMerge configs.
  • Style and refactor the Tabs component's public props (it now accepts a tags prop with the child tabs definition).
  • Fix Avatar component while displaying images.
  • Remove unused Chip props.
  • Remove Popover portal from Autocomplete components.
  • Include Tooltip.Portal; there were instances when the tooltip content became invisible.

How Has This Been Tested?

Manually

@leopuleo leopuleo self-assigned this Jan 15, 2025
Copy link
Member

@adrians5j adrians5j left a comment

Choose a reason for hiding this comment

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

1. Orange Border Height

I noticed this is 3px in Figma, and in code it's 2px.
image

But I think you already posted a comment to K about this, right?

2. Not sure if this should be in line.

image

3. Active States

Should we discuss these with K maybe?
image

image

4. Tabs Moving?

Isn't it weird how the right tab is moving by 1px as the left one is switching between active/inactive?

movingpx.mov

@adrians5j
Copy link
Member

adrians5j commented Jan 22, 2025

Note

At this point, this is just a discussion.

BTW what do you think about the following API here?

<Tabs>
    <Tabs.Tab id="account" title="Account" icon={<Tabs.Tab.Icon icon={...} label={...}/>}>Account content</Tabs.Tab>
    <Tabs.Tab id="password" title="Password">Password content</Tabs.Tab>
</Tabs>

Some notes...

  1. The current Tabs component uses tabs prop and has no children being used. Since both of these are just regular props, and both can be affected via a decorator, maybe might as well use children here? It'll also be a bit more in the spirit of React.
  2. In this example, I'm using Tabs.Tab instead of POJO. Note that both Tabs and Tabs.Tab can also be made as decoratable, giving users full control.
  3. Using Tabs.Tab.Icon not only allows the user to specify the icon, but also other props. Like in this example, a label for the icon, if needed. This is how I also did it with Dialog and it's icon prop. You pass Dialog.Icon element to it.
  4. BTW I used id instead of value. value sounds a bit weird to me... but hey, if it's how it works in the React/components world, I guess we leave it as is.

The only potentially tricky thing I can see here is the fact that the title needs to be extracted from the child components, by the Tabs parent component, in order to construct triggers. I can see where doing this already, but I can also see it's easier to do this when you have tabs POJO prop, and not these child components (although I believe it's still possible to read props from these child components, I think I did it in the past).

@leopuleo
Copy link
Contributor Author

All the styling issues highlighted above have been solved or discussed with K.

tabs prop

I've changed from POJO to the Tabs.Tab component. We need some little magic under the hood to extract all the valuable props for the underlying Trigger and Content components. The exact mechanism is used today.

This component is a good candidate for improvements via FTA; I noted it in the GH project.

Regarding the tabs vs children props: let's revisit them together with the other components.

tab icon

Currently, the icon prop receives any ReactElement, aka any SVG. We use the Icon component inside the Trigger where we pass the SVG received as a prop with fixed size, color and label.

I would keep it like this: I don't want to repeat all these fixed props while using this component around the admin.

id vs value prop

I chose to use value instead of id because this is what Radix UI uses, together with onValueChance callback. Also, the id prop is used internally by Radix UI to couple the content and the trigger.

CleanShot 2025-01-22 at 15 35 14@2x

@leopuleo leopuleo merged commit 895d2a9 into feat/new-admin-ui Jan 22, 2025
16 checks passed
@leopuleo leopuleo deleted the leo/fix/ui-checks branch January 30, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants