Skip to content

Complete Implementation for Error Display and Error Correction option - #68

Open
ajaynairdev wants to merge 2 commits into
developfrom
input-error-display
Open

Complete Implementation for Error Display and Error Correction option#68
ajaynairdev wants to merge 2 commits into
developfrom
input-error-display

Conversation

@ajaynairdev

Copy link
Copy Markdown
Collaborator

Fully Implemented the error display and also allow users to correct their errors as required.

@ishraqn

ishraqn commented Apr 21, 2024

Copy link
Copy Markdown
Owner

Initial Thoughts:

  • Seems like there is a lot of function calls in the back and frontend, could be optimised further.
  • The Styling could be improved since it doesnt match the other tabs. Some issues from one try:
    • Remove the 'x' button,
    • The box of the errors need to be within the same container like the other tabs.
    • The box goes over the menu text and other tabs (not self contained)
    • The sidebar doesnt expand to fit the long list.
    • The submit button needs to match the site.
  • The errors are not accurate/ not enough edge cases, some issues:
    • N/A, N/D, NA, ND, 0, CARUID == 0, Province and more (check the properties in geoSpatialJoin for domain knowledge)
    • Needs to be robust to handle case i.e upper lower.
    • I am not sure if samples being 0 counts as an error but could be.
    • The row number is one more than the row being viewed. I believe its the headers being counted as 1.
  • Remove the use of status 422.
  • There is a bug where it can cause the app to crash due to sending multiple error status codes. Refer to how other functions are calling errors, i believe its mostly through the console.error.
  • csv-parser is a new dep but it isnt in the package.json.

Note: Since you were taking some time, I actually went ahead and finished a lot of the work before this PR was published. Lmk if you want to carry it on.

@ajaynairdev

Copy link
Copy Markdown
Collaborator Author

Hey, i implemeted your recommendations. If its possible, take a look and met let know what you think.

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.

2 participants