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: fixed testset columns naming conventions #1743

Merged
merged 5 commits into from
Jun 4, 2024
Merged

fix: fixed testset columns naming conventions #1743

merged 5 commits into from
Jun 4, 2024

Conversation

ashrafchowdury
Copy link
Collaborator

@ashrafchowdury ashrafchowdury commented Jun 2, 2024

Description

Fixed the test set colums naming conventions by making it lowercase.

Screenshot

image

Issue

Closes #1737

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 2, 2024
Copy link

vercel bot commented Jun 2, 2024

@ashrafchowdury is attempting to deploy a commit to the mmabrouk's projects Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot bot added bug Something isn't working Frontend labels Jun 2, 2024
Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks @ashrafchowdury
The goal of the issue is not to set everything to lower case but just keep it as is

Copy link

vercel bot commented Jun 2, 2024

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

Name Status Preview Comments Updated (UTC)
agenta ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 4, 2024 10:40am

Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

I don't understand the changes that you have made and why you have made them..

The way I would tackle this is the following: Somewhere in the code we are transforming the input made by the user and uppercasing the first letter (unless this is the default behavior in ant.design which is very likely), you need to find that part and remove it. That's it.

agenta-web/src/components/TestSetTable/TestsetTable.tsx Outdated Show resolved Hide resolved
@ashrafchowdury
Copy link
Collaborator Author

The primary issue is with the ag-grid-react library used to generate the test set tables. This library does not allow styling of the column header names.The library, by default, uses CamelCase format for column headers.

Screenshot_3-6-2024_10393_www ag-grid com

To fix this issue, I am using the column names that are coming from the database, rather than the names provided by the library.

Code Changes:

  • Using the inputValues as header name which is comming from the database insead of displayName which is provided by the ag-grid-react library.
  • Implemented a setTimeout function to prevent the glitch associated with name changing. The glitch is between previous name and the new name.

@ashrafchowdury ashrafchowdury requested a review from mmabrouk June 3, 2024 06:14
@ashrafchowdury
Copy link
Collaborator Author

I don't understand the changes that you have made and why you have made them..

The way I would tackle this is the following: Somewhere in the code we are transforming the input made by the user and uppercasing the first letter (unless this is the default behavior in ant.design which is very likely), you need to find that part and remove it. That's it.

The code is fine, the uppercase formatting is a characteristic of the ag-grid-react library that are used to create the testset tables. This library by default CamelCase column headers.

ag-grid-react library doc: https://www.ag-grid.com/react-data-grid/column-headers/#header-name

@mmabrouk
Copy link
Member

mmabrouk commented Jun 3, 2024

The primary issue is with the ag-grid-react library used to generate the test set tables. This library does not allow styling of the column header names.The library, by default, uses CamelCase format for column headers.

Screenshot_3-6-2024_10393_www ag-grid com

To fix this issue, I am using the column names that are coming from the database, rather than the names provided by the library.

Code Changes:

  • Using the inputValues as header name which is comming from the database insead of displayName which is provided by the ag-grid-react library.
  • Implemented a setTimeout function to prevent the glitch associated with name changing. The glitch is between previous name and the new name.

Thanks for the explanation and for sharing the link to the documentation. I think you misunderstood the documentation. See the example that they provide, if the user specifies field they would use a camel case, if the user provides a headerName they use it as is. I think we just need to use headerName and that would resolve the issue, no?

CleanShot 2024-06-03 at 08 30 08@2x

@mmabrouk mmabrouk requested a review from bekossy June 3, 2024 06:32
Copy link
Member

@bekossy bekossy left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @ashrafchowdury
Following the Docs, in order to fix the naming convention you just need to set the headerName prop in the columnDefs object

For instance

            const newColDefs = [
                CHECKBOX_COL,
                ...colData.map((col) => ({
                    ...col,
                    headerName: col.field,
                })),
                ADD_BUTTON_COL,
            ]
            setColumnDefs(newColDefs)
            

You need to do this when we apply the columns, update the table and add new columns

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jun 3, 2024
@ashrafchowdury ashrafchowdury requested a review from bekossy June 3, 2024 07:18
@ashrafchowdury
Copy link
Collaborator Author

I apologize for everything @mmabrouk, I misunderstood the documentation. Also, thank you @bekossy, for the direction.

@bekossy
Copy link
Member

bekossy commented Jun 3, 2024

Awesome work @ashrafchowdury however we also need to do the same when we add a new column

Copy link
Member

@bekossy bekossy left a comment

Choose a reason for hiding this comment

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

Great work @ashrafchowdury

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 3, 2024
Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks @ashrafchowdury

For some reason the behavior is weird. If you use an upper case column name, it would be transformed to lower case, then if you try to edit it, it would be shown as upper case. I think we need to be consistent here. Either, we show exactly what the user has provided (upper case or lower case), or we transform everything to lowercase and show it as such.

CleanShot.2024-06-03.at.14.38.08.mp4

Second problem is that each time you make a modification to the test set and save, you create a new test set it appears! @bekossy can you please look into this. I did not see this issue in cloud but I am seeing it in the Vercel preview here, is this a new issue introduced by this PR?

@dosubot dosubot bot removed the lgtm This PR has been approved by a maintainer label Jun 3, 2024
@bekossy
Copy link
Member

bekossy commented Jun 3, 2024

@mmabrouk I don’t think the Vercel Preview is showing the latest changes in the PR as it was last deployed 17 hours ago

Screen.Recording.2024-06-03.at.1.55.28.PM.mov

Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks @bekossy
I deployed now the latest version. The issue now is that you cannot rename a column named 'correct_answer' to 'Correct_answer' although these are two different column names (since we now differ from lower case and upper case)

@bekossy bekossy requested a review from mmabrouk June 4, 2024 07:25
Copy link
Member

@mmabrouk mmabrouk left a comment

Choose a reason for hiding this comment

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

Thanks @ashrafchowdury and @bekossy ! Great work!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 4, 2024
@mmabrouk
Copy link
Member

mmabrouk commented Jun 4, 2024

@all-contributors please add @ashrafchowdury for bug fix and code

Copy link
Contributor

@mmabrouk

I've put up a pull request to add @ashrafchowdury! 🎉

@mmabrouk mmabrouk merged commit 8df17b2 into Agenta-AI:main Jun 4, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Frontend lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AGE-278] Show the name of the columns in the test set without upper case the first letter
3 participants