OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

Style improve fe layout and interaction

Open iFurySt opened this issue 1 year ago • 11 comments

Xnip2024-03-30_17-31-57 Xnip2024-03-30_17-31-46 Xnip2024-03-30_17-57-42 image Xnip2024-03-30_17-57-56 Xnip2024-03-30_17-57-52 Xnip2024-03-30_17-57-47

iFurySt avatar Mar 30 '24 10:03 iFurySt

this PR includes:

  1. extract the global style vars for consistent style.
  2. Integrate configuration in the same place(Setting Modal). And cache the user configuration in the local storage (user doesn't need to select every time)
  3. unify the color, especially the bg color.
  4. change almost all .css to Tailwind.

iFurySt avatar Mar 30 '24 10:03 iFurySt

👍 Will take a closer look, maybe need to resolve the conflict.

huybery avatar Mar 30 '24 12:03 huybery

This looks fantastic! A few comments:

  1. We should reduce the number of UI libraries. We currently use DaisyUI and this PR adds both NextUI and Ant Design. Using 3 UI libraries reduces consistency and increases bundle size. I don't think we use a lot of DaisyUI so the easiest way to do this might be to remove DaisyUI and use NextUI components instead.
  2. We'll want cache invalidation when the list of models/agents changes.
  3. IMO the settings component takes too many props.

(2) and (3) are low-priority but we should really address (1)

agree, we should keep as few third-party dependencies as possible.

huybery avatar Mar 30 '24 12:03 huybery

agree, i've just thought the daisyui is too "raw" to me~ so I import the nextui and antd for quickly dev, but i think keep one is okay, cuz i like the input of nextui and select of antd 😅

iFurySt avatar Mar 30 '24 13:03 iFurySt

This looks fantastic! A few comments:

  1. We should reduce the number of UI libraries. We currently use DaisyUI and this PR adds both NextUI and Ant Design. Using 3 UI libraries reduces consistency and increases bundle size. I don't think we use a lot of DaisyUI so the easiest way to do this might be to remove DaisyUI and use NextUI components instead.
  2. We'll want cache invalidation when the list of models/agents changes.
  3. IMO the settings component takes too many props.

(2) and (3) are low-priority but we should really address (1)

  1. yes, as i said above, i think nextui more faster for developing. it makes sense to switch to nextui.
  2. sure, i just realized a basic function.
  3. haha, i also think so, i can move these inside to components.

iFurySt avatar Mar 30 '24 13:03 iFurySt

@yimothysu i've just updated the PR, modified includes:

  1. remove daisyui, switch to nextui and antd;
  2. reduce the props of Workspace component;
  3. adjust style of chat bubble and workspace tabs.

Screenshot is here: Xnip2024-03-30_22-38-23 Xnip2024-03-30_22-38-33

iFurySt avatar Mar 30 '24 14:03 iFurySt

At first glance, it doesn't seem like we should be using both nextui and antd. Is there any reason you can't use just one of those?

rbren avatar Mar 30 '24 14:03 rbren

(UI improvements look great btw!)

rbren avatar Mar 30 '24 14:03 rbren

At first glance, it doesn't seem like we should be using both nextui and antd. Is there any reason you can't use just one of those?

yes, but as I said before, the antd has more convenient components that nextui doesn't have, which can let us iterate more faster. But we can convergence in the future, I think it's priority not too high right now

iFurySt avatar Mar 30 '24 14:03 iFurySt

Having been through this before--it's very hard to rip out a UI framework once people start using it.

I defer to the frontend folks, but IMO we should standardize on a single component framework from day 1

rbren avatar Mar 30 '24 16:03 rbren

Having been through this before--it's very hard to rip out a UI framework once people start using it.

I defer to the frontend folks, but IMO we should standardize on a single component framework from day 1

I completely agree with @rbren's suggestion about using a single UI library from day one. What do you think about adopting shadcn? It integrates seamlessly with Tailwind, which we're already using, making it an excellent pairing.

akhilvc10 avatar Mar 30 '24 21:03 akhilvc10

i removed the antd, currently all in nextui

Xnip2024-03-31_10-20-25

iFurySt avatar Mar 31 '24 02:03 iFurySt