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

Replace angular.fromJson with JSON.parse #7952

Merged
merged 4 commits into from
Apr 14, 2020
Merged

Replace angular.fromJson with JSON.parse #7952

merged 4 commits into from
Apr 14, 2020

Conversation

BatJan
Copy link
Contributor

@BatJan BatJan commented Apr 11, 2020

Prerequisites

Fixing this issue https://trello.com/c/uPQwJmDQ/9-replace-instances-of-angularfromjson

If there's an existing issue for this PR then this fixes

Description

I have been replacing all instances of angular.fromJson with JSON.parse. 😃

@nul800sebastiaan nul800sebastiaan merged commit a5ff8a4 into umbraco:v8/contrib Apr 14, 2020
@nul800sebastiaan
Copy link
Member

Thanks! 👍

@bjarnef
Copy link
Contributor

bjarnef commented Apr 27, 2020

It seems this is causing issues now, when inserting e.g. image grid editor in grid content.

return JSON.parse(storage["umb_" + key]);

where angular.fromJson() is a bit different:
https://github.com/angular/angular.js/blob/master/src/Angular.js#L1335-L1339

With angular.fromJson() I can open the image dialog:

image

with just the current change using JSON.Parse():
image

It seems to be an empty when session storage is empty, where JSON.Parse throw an error.

@nul800sebastiaan
Copy link
Member

@bjarnef Hmmm.. Maybe it's the same issue as #8013 though? And not so much an issue with this change?

@bjarnef
Copy link
Contributor

bjarnef commented Apr 27, 2020

@nul800sebastiaan not sure if the issue @lars-erik is having is the same since it is in v8.6.x ...
anyway I am working on a PR regarding the issue, when inserting an image grid editor in grid content:
#8014

@lars-erik
Copy link
Contributor

@nul800sebastiaan @bjarnef This is not related to #8013, but makes it so you can't even get to #8013. 😆
I didn't notice since I copied the change to the controller into built output in a soon-to-be-prod-ready 8.6.1 site, so didn't get the services update that blocks it. Now doing more work for a possible second PR based on the contrib branch where I merged #8014 and about to test more.

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.

5 participants