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

Update Admin.js - fix edit list field inline mode triggering validation #6569

Closed
wants to merge 2 commits into from

Conversation

leoBergerot
Copy link

Fix edit list field inline mode triggering validation.

I am targeting this branch, when you edit field inline mode when validation is triggering there is encoding problems.

Before my solution:
Screenshot_2020-11-04 Admin - Stock List(1)
After:
Screenshot_2020-11-04 Admin - Stock List

Fix edit inline validation, encoding problems
@leoBergerot leoBergerot changed the title Update Admin.js Update Admin.js - Fix edit list field inline mode triggering validation Nov 4, 2020
@leoBergerot leoBergerot changed the title Update Admin.js - Fix edit list field inline mode triggering validation Update Admin.js - fix edit list field inline mode triggering validation Nov 4, 2020
core23
core23 previously approved these changes Nov 4, 2020
@core23 core23 added the patch label Nov 4, 2020
@@ -226,6 +226,10 @@ var Admin = {
.replaceWith(html);
},
error: function(xhr, statusText, errorThrown) {
if (xhr.status === 400){
Copy link
Member

Choose a reason for hiding this comment

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

Why only for 400 ?

Do you know if we have to JSON.parse for every case ?
If not, I think a comment could help to understand this check.

Copy link
Author

Choose a reason for hiding this comment

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

Yes indeed JSON.parse is also needed for 405, 404, 403, i will update this in my pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Is there some case where you don't need JSON.parse ?

If there are not, we can just write

error: function(xhr, statusText, errorThrown) {
    return JSON.parse(xhr.responseText);
}

If there are, how did you decide (between using JSON.parse) for them and for 400, 403, 404, 405 ?

Copy link
Author

Choose a reason for hiding this comment

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

No because if is a 500 throwed you don't see it, it's html not Json

Copy link
Author

Choose a reason for hiding this comment

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

ahhh you are right, in production it's not a problem… i speak in case of dev

@@ -226,6 +226,10 @@ var Admin = {
.replaceWith(html);
},
error: function(xhr, statusText, errorThrown) {
if (xhr.status === 400 || xhr.status === 403 || xhr.status === 404 || xhr.status === 405){
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we trust the content-type header instead?

Copy link
Member

Choose a reason for hiding this comment

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

That would be great to have a better check than status code indeed.

Copy link
Member

Choose a reason for hiding this comment

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

No news from @leoBergerot, do you know how to update the PR in order to use the content-type header @phansys ?

Copy link
Member

Choose a reason for hiding this comment

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

See #6856.

@VincentLanglet
Copy link
Member

Hi @leoBergerot, Do you have some time to update the PR ?

You'll also need to rebase 3.x in order to fix the build ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants