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.

Sign up for GitHub

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

Jump to bottom

chore: Add checkbox to clear all Variables #666

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

chore: Add checkbox to clear all Variables #666

wants to merge 11 commits into from

Conversation

sangwoo-joh
Copy link
Contributor

@sangwoo-joh sangwoo-joh commented Apr 13, 2024 •
edited
Loading

This fix is a suggestion that addresses a perhaps personal inconvenience for me. If you have a better solution, I'd love to hear it and hope you'll consider this positively.

The CSV file I use for testing has a lot of columns, not all of which are actually used in the prompt template, but all of which appear as Variable in the viewer. This makes it difficult to keep only the columns I want in Select.
This fix adds a checkbox to either deselect all the Variables at once or select them all. I couldn't figure out the right place for it, so I put it in the same Box as the column Select for now, but if you have a better place, please let me know.

Thanks for the great open source. I always appreciate it!

@typpo
Copy link
Contributor

typpo commented Apr 15, 2024 •
edited
Loading

Great change! I think my preference here would be to create a dialog that allows for selection of columns, in order to not clutter the top space with many options. What do you think?

@sangwoo-joh
Copy link
Contributor Author

@typpo I hadn't thought of a dialogue, but that sounds like a good idea. I'll give it a try.

@sangwoo-joh sangwoo-joh force-pushed the clear-all-variables branch from d11140d to 49be9ac Compare April 16, 2024 00:59
@sangwoo-joh
Copy link
Contributor Author

Dear @typpo, I replaced the Select panel of columns with a Dialog and put the "Clear All Variables" button at the top of it. It seemed to work when I tested it locally. I'm not good at designing, so I picked moderate buttons. I'd appreciate it if you could review this!

@sangwoo-joh
Copy link
Contributor Author

I tried to upload some working images and gifs, but they won't upload due to Github's issue...

typpo
typpo reviewed Apr 16, 2024
View reviewed changes
Copy link
Contributor

@typpo typpo left a comment

Choose a reason for hiding this comment

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

This is coming together, thanks for setting up the dialog! If it's ok with you, I'll do a pass on the design/layout probably tomorrow. Then I'm happy to merge :)

src/web/nextui/src/app/eval/ResultsView.tsx Outdated
Comment on lines 312 to 313
<Checkbox checked={selectedColumns.indexOf(column.value) > -1} />
<ListItemText primary={column.label} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm noticing that clicking the checkbox should update the state (right now, you can't actually toggle the checkbox). Let's also connect the label with the checkbox, so that clicking it toggles the state too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've noticed too. I'm trying to fix this now. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@typpo I've restored the original functionality of the checkbox.
Please let me know if I have additional corrections.
Thank you!

@sangwoo-joh sangwoo-joh force-pushed the clear-all-variables branch from 4551f63 to efa752b Compare April 16, 2024 04:44
@sangwoo-joh
Copy link
Contributor Author

I rebased these commits to the main and then force-pushed them, out of habit. If that's not the policy of this repository, please let me know and next time I'll resolve conflicts.

@mldangelo mldangelo force-pushed the main branch 2 times, most recently from 99b5c63 to 790ec30 Compare June 8, 2024 19:41
@typpo typpo force-pushed the main branch 9 times, most recently from 395b5af to 54ba13e Compare June 13, 2024 09:51
@typpo typpo force-pushed the main branch 2 times, most recently from b2323de to 3ad344f Compare June 18, 2024 18:36
@typpo typpo force-pushed the main branch 6 times, most recently from bb2ca8e to 50b943f Compare July 1, 2024 17:08
@mldangelo mldangelo changed the title Add checkbox to clear all Variables chore: Add checkbox to clear all Variables Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@typpo typpo typpo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sangwoo-joh @typpo