umco / umbraco-inner-content

A helper library for Umbraco Doc Type based property editors providing overlays and conversion helpers
https://our.umbraco.org/projects/backoffice-extensions/inner-content/
MIT License
2 stars 15 forks source link

Copied content and paste button stick around too long #51

Closed dnwhte closed 5 years ago

dnwhte commented 5 years ago

When a stacked content item is copied it gets added to Local Storage, but is never removed. Since local storage does not expire this leads to a confusing UX. An editor may come back to editing a page days or weeks after copying a stacked content and still see the paste button, which leads to confusion.

I would propose either:

  1. Clear the copied item after it is pasted for the first time. If the user wanted more than one copy they would need to recopy.

  2. Only keep the copied item around for the current Umbraco session.

  3. A clear copied button. Could be more confusing since it doesn't mimic any other copy/paste experience.

Any thoughts? I'd be happy to do a PR.

leekelleher commented 5 years ago

@dnwhte Thanks for the feedback!

Are you thinking sessionStorage instead of localStorage?

With clearing the copied item after the first time, I'd considered how operating systems handle it, generally when you copy to the clipboard, it can be pasted multiple times.

leekelleher commented 5 years ago

I recall why we used localStorage over sessionStorage, it was so we could copy/paste between browser-tabs. sessionStorage is restricted to the specific browser-tab. Unless if there was a clever way to get around that restriction? 🤔

dnwhte commented 5 years ago

I like localStorage, but just need some trigger to clear it on. Are there Umbraco login/logout events on the client side?

Another issue with the way it currently is, an editor could paste content from a node they do not have access if they are using the same machine as the editor that had copied it.

dnwhte commented 5 years ago

What about on app.ready? https://github.com/umbraco/Umbraco-CMS/blob/91c52cffc8b7c70dc956f6c6610460be2d1adc51/src/Umbraco.Web.UI.Client/src/init.js#L41

dnwhte commented 5 years ago

nvm, replying on umbraco events is not reliable.

What about just expiring it in an hour? Add a timestamp in the localStorage and use it to check if time has passed.

leekelleher commented 5 years ago

I'm swaying towards using sessionStorage, feels the appropriate method for handling this, (and I could compromise on the cross-tabs support). @mattbrailsford Any thoughts on this?

mattbrailsford commented 5 years ago

sessionStorage is fine with me. Feels it provides more benefits than negatives.

dnwhte commented 5 years ago

v7 uses an old version of angular-local-storage which does not allow you to dynamically set the storage type. So we'd have to use the OOTB sessionStorage browser API. Might make the code a bit more brittle or verbose. Is that fine? I can proceed with a PR.

FYI, v8 uses the latest version of angular-local-storage.

dnwhte commented 5 years ago

I added a PR using sessionStorage (#52). It contains a get a set method for working with sessionStorage. The downside is it won't fallback to cookies like localStorageService.

leekelleher commented 5 years ago

Cool, thanks @dnwhte! 🎉 I'll close off this ticket, we can discuss the implementation on PR #52!