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

feat: add combobox for export options #292

Merged
merged 7 commits into from
Apr 21, 2023

Conversation

Jshen123
Copy link
Contributor

@Jshen123 Jshen123 commented Apr 21, 2023

Description:
Export buttons are now grouped under one dropdown
image
other

  • added Menu component

@vercel
Copy link

vercel bot commented Apr 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
agent-gpt ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 21, 2023 6:40am

@asim-shrestha
Copy link
Contributor

Nice,

we should ensure hover effects trigger on the entire group. Looks like hovering the drop down icon doesn't trigger the glow. Tailwind has a group handler we can use for this.

We should also be able to click anywhere on the button. Looks like only the dropdown itself works currently

@asim-shrestha
Copy link
Contributor

Nice,

we should ensure hover effects trigger on the entire group. Looks like hovering the drop down icon doesn't trigger the glow. Tailwind has a group handler we can use for this.

We should also be able to click anywhere on the button. Looks like only the dropdown itself works currently

This might have to do with the fact that the other combobox is editable so some inconsistency here. TBH we probably don't even want it to be editable

@@ -167,6 +168,35 @@ const MacWindowHeader = (props: HeaderProps) => {
void navigator.clipboard.writeText(text);
};

const exportOptions = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we extract an ExportComboBox element?

@Jshen123
Copy link
Contributor Author

Jshen123 commented Apr 21, 2023

Hi @asim-shrestha, reimplemented the dropdown with the new menu component.

  • button text should no longer be editable
  • hover on chevron should trigger hover effect
    image

@awtkns
Copy link
Contributor

awtkns commented Apr 21, 2023

Nice work here! Thoughts on keeping the save to db button outside the ddl? In doing that we could then rename it it to export.

@@ -167,8 +168,34 @@ const MacWindowHeader = (props: HeaderProps) => {
void navigator.clipboard.writeText(text);
};

const exportOptions = [
<WindowButton
key="Agent"
Copy link
Contributor Author

@Jshen123 Jshen123 Apr 21, 2023

Choose a reason for hiding this comment

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

@awtkns, I grouped save agent button under the dropdown menu.

  • it will be disabled unless onSave is not undefined

Copy link
Contributor

@awtkns awtkns left a comment

Choose a reason for hiding this comment

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

LTGM, my one suggestion would be to tighten up the padding on the save button, not sure it needs to quite so big.

@Jshen123
Copy link
Contributor Author

Nice work here! Thoughts on keeping the save to db button outside the ddl? In doing that we could then rename it it to export.

I prefer it outside as well since it will be more obvious to the users.

It's mostly a responsive thing. not enough space for both when the screen is small. but I just have an idea now. taking a look

@asim-shrestha
Copy link
Contributor

We could also do a ping animation in the DDL for when export to DB is available: https://tailwindcss.com/docs/animation#ping

@Jshen123
Copy link
Contributor Author

Jshen123 commented Apr 21, 2023

@asim-shrestha @awtkns not what i had in mind an hour ago but i have separated out the Save agent button.

It's closer to what we have in production.

image

@awtkns
Copy link
Contributor

awtkns commented Apr 21, 2023

@Jshen123 not sure why merging is blocked... i can force merge it for you if you want

@asim-shrestha asim-shrestha merged commit 39306a9 into main Apr 21, 2023
3 checks passed
@asim-shrestha asim-shrestha deleted the feat/add_combobox_for_export branch May 18, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants