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

Add additional header row with letters #436

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

Conversation

guergana
Copy link
Collaborator

@guergana guergana commented Jun 22, 2024

Copy link

cloudflare-workers-and-pages bot commented Jun 22, 2024

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: 14b249c
Status: ✅  Deploy successful!
Preview URL: https://14f6e2f6.opendataeditor.pages.dev
Branch Preview URL: https://429-add-letters-header.opendataeditor.pages.dev

View logs

@pdelboca
Copy link
Member

@guergana we will need to find a way to do AA, AB, AC, AD when having more columns than the alphabet of letters:

image

@guergana
Copy link
Collaborator Author

guergana commented Jun 24, 2024

@guergana we will need to find a way to do AA, AB, AC, AD when having more columns than the alphabet of letters:

image

Thanks for the heads up @pdelboca , I have already wrote a comment to @romicolman to ask what the format will be. But this PR covers the research part I think. :) we could either close this PR and make a new task with the complete changes or maybe continue the task on this PR?

To me the format A1, B1 looks more intuitive than AA. But I am not very aware of how this works in other programs. How does excel do it?

@romicolman
Copy link
Collaborator

Hi! I created a new ticket to request changes (#441) but we can use this PR for the implementation.

If possible, let's keep the same logic that we see in Google Sheets, Excel: A-Z and then 'AA', 'AB', 'AC', 'AD'. In this sense, since we use numbers for rows, in the error report we can be more specific: errors in cells AA1, B53, etc.

@guergana
Copy link
Collaborator Author

Hi! I created a new ticket to request changes (#441) but we can use this PR for the implementation.

If possible, let's keep the same logic that we see in Google Sheets, Excel: A-Z and then 'AA', 'AB', 'AC', 'AD'. In this sense, since we use numbers for rows, in the error report we can be more specific: errors in cells AA1, B53, etc.

Ok..thanks for the clarification. :)

@romicolman
Copy link
Collaborator

@pdelboca this is the PR where we need to check Frictionless to see to what extent we can tailor the errors report:
https://framework.frictionlessdata.io/docs/errors/resource.html

@guergana guergana marked this pull request as draft June 24, 2024 19:25
@guergana guergana marked this pull request as ready for review June 24, 2024 20:20
@guergana
Copy link
Collaborator Author

guergana commented Jun 24, 2024

Can be tested with the attached file.
CSV_Data_2024_6_24 22 25 many columns.csv

Or you can generate one here: https://markbdsouza.github.io/csv-data-generator/

@guergana guergana requested review from roll and pdelboca June 24, 2024 20:30
@romicolman
Copy link
Collaborator

Hi! I used the cvs shared in this PR and other csvs to test the change on Mac. It works OK.
@pdelboca Can you check it in Windows?

@pdelboca
Copy link
Member

@romicolman @guergana tried on windows and I didn't encounter any error! Works as expected!

I do think that so far it adds more noise than help, so I would make sure that it is possible and feasible to integrate this with other features of ODE so we don't leave this extra metadata without any use (I think the idea is to use this in the report).

@guergana
Copy link
Collaborator Author

@romicolman @guergana tried on windows and I didn't encounter any error! Works as expected!

I do think that so far it adds more noise than help, so I would make sure that it is possible and feasible to integrate this with other features of ODE so we don't leave this extra metadata without any use (I think the idea is to use this in the report).

I agree. We need to test already if this can be integrated in the report because here it is only part of the table rendering. You are absolutely right, @pdelboca I didn't try to add it to the report because @romicolman said these changes will come later..but we need to find a way to at least test if we can send the values..

@romicolman
Copy link
Collaborator

romicolman commented Jul 22, 2024

Hey! Sorry for my late response. Yes, we are adding because we need it in order to improve the errors report.
The idea is that we can have a proper errors report before the pre-release.

Can we close this PR now? I mean, let's implement the change, please

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.

Improve datagrid to help the user read errors Improve datagrid to help the user read errors - Research
3 participants