Copy & Paste feature #44
Conversation
- Replaced the `window.localStorage` calls with the `localStorageService` (which is used by Umbraco core and has built-in fallbacks) - Replaced emptying the "key" and removing the "$$hashKey" properties to the `stringify` function - Reworked the `canPaste` function, as it was being evaluated several times, when the value could be referenced in a local field variable - Reduced the "Is Content Validate" function, it was over-working - Set the dirty flag, after pasting content - Added (temporary) TODO notes
| $scope.canAdd = function () { | ||
| return (!$scope.model.config.maxItems || $scope.model.config.maxItems === 0 || $scope.model.value.length < $scope.model.config.maxItems) && $scope.model.config.singleItemMode !== "1"; | ||
| return (!$scope.model.config.maxItems || $scope.model.config.maxItems === "0" || $scope.model.value.length < $scope.model.config.maxItems) && $scope.model.config.singleItemMode !== "1"; |
There was a problem hiding this comment.
Is this always going to be string? Should we intParse then comapre?
There was a problem hiding this comment.
I believe that prevalues will be strings. This wasn't an issue previously, as we were using double-equals, so the JS was evaluating it. I'd tried to follow the ESLint rules by using the triple-equals, but the value was a string (based on my tests).
|
|
||
| $scope.canPaste = function () { | ||
| var stackedContentItem = JSON.parse(window.localStorage.getItem("StackedContentCopy")); | ||
| if (stackedContentItem && validateModel(stackedContentItem)) return true; | ||
| if ($scope.canCopy() && $scope.canAdd()) { |
There was a problem hiding this comment.
Maybe canAdd should include a ls.isSupported check so you only need to check canAdd? Seems odd to check canCopy
There was a problem hiding this comment.
My thinking was that you can't paste without copying it first. But yeah, we can replace it with isSupported.
| return; | ||
| $scope.copyContent = function (evt, idx) { | ||
| // TODO: Move this to InnerContent Service | ||
| var item = $scope.model.value[idx]; |
There was a problem hiding this comment.
Does this need an ls.isSupperted check?
There was a problem hiding this comment.
I'd thought about it, but the copy feature wouldn't be available if canCopy was false.
Unless if a dev were to hack the API themselves?
| } | ||
| return v; | ||
| })); | ||
| allowPaste = true; |
There was a problem hiding this comment.
I'm not convinced the allowPaste local variable is fool proof. Is there as possibility this could be false/true but there is something pastable int he clipboard? How expensive is the pastedAllowed() call?
There was a problem hiding this comment.
It felt pretty expensive. I'd added a console.log in canPaste - I had 5 StackedContent fields on a page, (with at least 10 blocks between them) - meaning it was being called for each field, then each row.
It felt overkill to get the data from localStorage, parse it and loop over the contentType GUIDs each time.
There's mostly a clever way to handle this?
| @@ -4,9 +4,10 @@ | |||
|
|
|||
| <div ng-show="inited"> | |||
|
|
|||
| <a ng-if="!model.value || model.value.length == 0" ng-click="addContent($event, 0)" class="placeholder" title="Add content"> | |||
| <div ng-if="!model.value || model.value.length == 0"ng-click="addContent($event, 0)" class="placeholder" title="Add content"> | |||
There was a problem hiding this comment.
Something is wrong here. This should be two links side by side, but it appears the paste link is inside the add link. Needs refactoring.
There was a problem hiding this comment.
I'd played around with various options - tried 2 <a> side-by-side, we'd lose the functionality to click anywhere inside the "Add content" panel - users would have to click the "icon-umb-contour" icon.
as localStorageService handles this (and angular.toJson removes the "$$haskKey" property)
|
Recently tested the feature branch for this and in my environment when you click the copy button, it causes the rendered preview to revert back to a simple icon and name template. This is most likely because you are copying the content by reference instead of just value. The code is a little goofy, but to avoid copying an object by reference you can stringify and then parse it. Doing this resolved the issue. My preview stays in tact. Otherwise, can you just re-render the preview? But it seems like that's extraneous. Please let me know if you can't reproduce. |
|
@markadrake - I've only been testing with previews disabled, so I hadn't noticed this, but thank you for spotting it - I'll take a look. |
Thanks to @markadrake for spotting this! #44 (comment)
|
@markadrake - cool, updated in commit: faf4aff - thanks again! |
|
Merging into the "dev-v2" branch. I've been using it for several weeks on client projects, it's pretty solid. |
Following on from merging PR #27 in, I've done some refactoring and refinements.
I've not moved anything over to the InnerContent Service API yet, I wanted us to be happy with how this is progressing. (I've marked the API points with
TODOcomments)