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

Complete Implementation for Error Display and Error Correction option #68

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ajaynairdev
Copy link
Collaborator

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

@ishraqn
Copy link
Owner

ishraqn commented Apr 21, 2024

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
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