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

Ajax code #976

Merged
merged 6 commits into from
Dec 10, 2023
Merged

Ajax code #976

merged 6 commits into from
Dec 10, 2023

Conversation

70ray
Copy link
Collaborator

@70ray 70ray commented Dec 2, 2023

Let ajax function return all data on not ok
so we can examine the error code if necessary.
Add a function ajaxAlert() to alert with the error message.

@70ray
Copy link
Collaborator Author

70ray commented Dec 2, 2023

Sandbox at: https://www.pgdp.org/~rp31/c.branch/ajax_code

The only place this could have any effect so far as I can tell is the page browser where selecting an invalid project id or non-existent page will give an alert message.

scripts/api.js Outdated Show resolved Hide resolved
@cpeel cpeel requested review from chrismiceli, cpeel and srjfoo December 4, 2023 04:38
Copy link
Member

@cpeel cpeel left a comment

Choose a reason for hiding this comment

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

Returning the full error block to the client makes sense. I'm a little wary of the convention of having the client alert() the error to the user. That means we need to design error messages for the end user, not the client developer.

@cpeel
Copy link
Member

cpeel commented Dec 4, 2023

Note: we should squash these commits on merge.

Copy link
Member

@srjfoo srjfoo left a comment

Choose a reason for hiding this comment

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

Bearing in mind that I know absolutely nothing about ajax, and that the code means nothing to me, I tested the page browser with invalid, nonexistent and archived projects, and with a nonexistent page number, comparing TEST with the branch, and didn't see any discrepancies.

scripts/api.js Outdated Show resolved Hide resolved
@chrismiceli
Copy link
Collaborator

Returning the full error block to the client makes sense. I'm a little wary of the convention of having the client alert() the error to the user. That means we need to design error messages for the end user, not the client developer.

I do think the ajaxAlert method is kind of a strange way to handle errors, though it is cheap.

@70ray
Copy link
Collaborator Author

70ray commented Dec 9, 2023

Sandbox updated with latest changes.

@cpeel cpeel merged commit d7a1767 into DistributedProofreaders:master Dec 10, 2023
4 checks passed
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.

4 participants