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

Add utility function equivalent to angular.fromJson #8014

Merged
merged 3 commits into from
May 6, 2020

Conversation

bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Apr 27, 2020

Prerequisites

  • I have added steps to test this contribution in the description below

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

Description

This previous PR #7952 replacing angular.fromJson from JSON.parse seems to fail as angular.fromJson also checked in the value passed in was equal to an string value.
https://github.com/angular/angular.js/blob/master/src/Angular.js#L1335-L1339

E.g. when inserting an image editor in grid content it is causing an console error, when session storage is empty.

Furthermore we now have a Utilities.toJson() function #7924 so I think we should have an Utilities.fromJson as well.

You can of course always use the traditional JSON.parse inside a try/catch block in e.g, but this method (similar to the angular.fromJson() only try to parse the value if it is an string value).

try {
    var obj = JSON.parse(value);
}
catch { 

}

@bjarnef
Copy link
Contributor Author

bjarnef commented Apr 27, 2020

Before
2020-04-27_13-09-06

After

2020-04-27_13-11-59

I do however get an YSOD when saving an content with empty image grid editor (or an selected image), but I think this is another underlying issue.

@bjarnef bjarnef marked this pull request as ready for review April 27, 2020 11:14
@bjarnef
Copy link
Contributor Author

bjarnef commented Apr 27, 2020

@nul800sebastiaan it seems there have been some other underlying issues regarding when having an image grid editor in grid and saving the content, but we can look at this in another PR.

@lars-erik
Copy link
Contributor

lars-erik commented Apr 27, 2020

I just hit the same issue and was about to fix that as well. Will merge this to test mine. When cropping, you hit another issue. See #8013. I wanna see about using the cropper from the imagecropper datatype instead as well, maybe in another PR tho.

@bjarnef
Copy link
Contributor Author

bjarnef commented Apr 27, 2020

The YSOD I get when saving grid content with image grid editor (or just a regular property using media picker) seems to be because I am missing some relations, e.g. umbMedia.
193892f

Maybe the upgrader didn't run as expected after I previous pulled some of the latest changes in v8/contrib branch.

@bjarnef
Copy link
Contributor Author

bjarnef commented Apr 27, 2020

I installed in clean Umbraco install and installed the starter kit. I now have the "Related Media" and "Related Content" relations. However I think these shouldn't have the "delete" actions since it would break stuff if these are deleted. Maybe these should be "locked" similar to default listview datatypes used in content, media and member?

image

I think bad things could happen, if these are deleted. I have reported a feature request for this here: #8021

@nielslyngsoe
Copy link
Member

This addition to Utilities is great! Thanks

@nielslyngsoe nielslyngsoe changed the base branch from v8/contrib to v8/dev May 6, 2020 14:37
@nielslyngsoe nielslyngsoe merged commit 88c842c into umbraco:v8/dev May 6, 2020
@bjarnef
Copy link
Contributor Author

bjarnef commented May 8, 2020

@nielslyngsoe shouldn't this be merged into v8/contrib which is this default branch now?

@nielslyngsoe
Copy link
Member

@nielslyngsoe shouldn't this be merged into v8/contrib which is this default branch now?

Yes and no — Normally I wouldn't be merging these PRs. But we needed it straight away so I just decided to merge it directly to the dev branch. The Dev branch should be merged back to contrib branch each day so hopefully, no one was waiting on this one. Hence no one did merge it from the contrib-team.

@bjarnef
Copy link
Contributor Author

bjarnef commented May 11, 2020

@nielslyngsoe yeah, I just needed this fix when submitting other PRs, but I had the change locally and only comitted the relevant changes for the specific feature/bug. It seems this is merged to v8/contrib now, so that's great 👍

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