Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Notable changes to this project are documented in this file. The format is based
Breaking changes:

New features:
- Remove `localStorage` for session storage, persist editor state in URL query param (#299 by @ptrfrncsmrph)

Bugfixes:

Expand Down
3 changes: 2 additions & 1 deletion client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
},
"dependencies": {
"ace-builds": "^1.5.0",
"jquery": "^1.12.4"
"jquery": "^1.12.4",
"lz-string": "^1.4.4"
}
}
7 changes: 7 additions & 0 deletions client/src/Try/Container.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,10 @@ export function setupIFrame(data, loadCb, failCb) {

return $iframe;
}

export function copyToClipboard(string, copyCb, failCb) {
navigator.clipboard.writeText(string).then(
() => copyCb(),
() => failCb()
);
}
68 changes: 47 additions & 21 deletions client/src/Try/Container.purs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import Data.String (Pattern(..))
import Data.String.Regex as Regex
import Data.String.Regex.Flags as RegexFlags
import Effect (Effect)
import Effect.Aff (Aff, makeAff)
import Effect.Aff (Aff, Milliseconds(..), delay, makeAff)
import Effect.Aff as Aff
import Effect.Class.Console (error)
import Effect.Uncurried (EffectFn3, runEffectFn3)
Expand All @@ -30,14 +30,14 @@ import Try.Editor (MarkerType(..), toStringMarkerType)
import Try.Editor as Editor
import Try.Gist (getGistById, tryLoadFileFromGist)
import Try.GitHub (getRawGitHubFile)
import Try.QueryString (getQueryStringMaybe)
import Try.Session (createSessionIdIfNecessary, storeSession, tryRetrieveSession)
import Try.QueryString (compressToEncodedURIComponent, decompressFromEncodedURIComponent, getQueryStringMaybe, setQueryString)
import Try.SharedConfig as SharedConfig
import Type.Proxy (Proxy(..))
import Web.HTML (window)
import Web.HTML.Window (alert)
import Web.HTML.Location (href)
import Web.HTML.Window (alert, location)

type Slots = ( editor :: Editor.Slot Unit )
type Slots = ( editor :: Editor.Slot Unit, shareButton :: forall q o. H.Slot q o Unit )

data SourceFile = GitHub String | Gist String

Expand Down Expand Up @@ -84,10 +84,11 @@ data Action
_editor :: Proxy "editor"
_editor = Proxy

type LoadCb = Effect Unit
type SucceedCb = Effect Unit
type FailCb = Effect Unit
foreign import setupIFrame :: EffectFn3 { code :: String } LoadCb FailCb Unit
foreign import setupIFrame :: EffectFn3 { code :: String } SucceedCb FailCb Unit
foreign import teardownIFrame :: Effect Unit
foreign import copyToClipboard :: EffectFn3 String SucceedCb FailCb Unit

component :: forall q i o. H.Component q i o Aff
component = H.mkComponent
Expand All @@ -109,8 +110,7 @@ component = H.mkComponent
handleAction :: Action -> H.HalogenM State Action Slots o Aff Unit
handleAction = case _ of
Initialize -> do
sessionId <- H.liftEffect $ createSessionIdIfNecessary
{ code, sourceFile } <- H.liftAff $ withSession sessionId
{ code, sourceFile } <- H.liftAff withSession

-- Load parameters
mbViewModeParam <- H.liftEffect $ getQueryStringMaybe "view"
Expand Down Expand Up @@ -141,12 +141,7 @@ component = H.mkComponent
handleAction $ Compile Nothing

Cache text -> H.liftEffect do
Copy link
Contributor Author

@pete-murphy pete-murphy Aug 27, 2022

Choose a reason for hiding this comment

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

Not sure if Cache still makes sense as the action name, maybe Persist? EncodeInURL?

Copy link
Contributor

Choose a reason for hiding this comment

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

EncodeInUrl sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed here 6eb73be

sessionId <- getQueryStringMaybe "session"
case sessionId of
Just sessionId_ -> do
storeSession sessionId_ { code: text }
Nothing ->
error "No session ID"
setQueryString "purs" $ compressToEncodedURIComponent text

Compile mbCode -> do
H.modify_ _ { compiled = Nothing }
Expand Down Expand Up @@ -340,6 +335,7 @@ component = H.mkComponent
]
[ HH.text "Show JS" ]
]
, HH.slot_ (Proxy :: _ "shareButton") unit shareButton unit
, HH.li
[ HP.class_ $ HH.ClassName "menu-item" ]
[ HH.a
Expand Down Expand Up @@ -442,6 +438,36 @@ renderCompilerErrors errors = do
, renderPlaintext message
]

shareButton :: forall q i o. H.Component q i o Aff
shareButton = H.mkComponent
{ initialState: \_ -> 0
, render
, eval: H.mkEval $ H.defaultEval
{ handleAction = handleAction
}
}
where
handleAction :: Unit -> H.HalogenM Int Unit () o Aff Unit
handleAction _ = do
url <- H.liftEffect $ window >>= location >>= href
H.liftAff $ makeAff \f -> do
runEffectFn3 copyToClipboard url (f (Right unit)) (f (Left $ Aff.error "Failed to copy to clipboard"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copying the pattern I saw with how setupIFrame is called, but maybe showing a message to the user is more appropriate than throwing an Error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a simple window >>= alert would be a cheap-and-easy way to notify the user? I know it's not the best UX, but do we anticipate this failing that often?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, since the "Share URL" changes to 'Copied to Clipboard" when that is clicked, could it also be changed to "Failed to Copy" when a failure occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, since the "Share URL" changes to 'Copied to Clipboard" when that is clicked, could it also be changed to "Failed to Copy" when a failure occurs?

Yeah, I'll plan on that 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Showing the error message in UI here 6eb73be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we anticipate this failing that often?

Browser support is pretty much universal https://caniuse.com/mdn-api_clipboard_writetext. I was thinking there was a possibility that some users might have disabled writing to the clipboard, but I haven't figured out how to do that in Chrome/Firefox. In Chrome there is a setting for disabling reading from clipboard, but haven't seen a way of limiting write access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I’d probably want to also wrap the FFI call in try/catch in the 5% chance that navigator.clipboard.writeText doesn’t exist (in non-supported browsers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here: 973b0ff

mempty
H.modify_ (_ + 1)
H.liftAff $ delay (1_500.0 # Milliseconds)
H.modify_ (_ - 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this state would be more clear if it were a showConfirmation :: Boolean instead of Int? I used Int so that repeated clicks would keep the confirmation message showing, but that's probably not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this block the thread because you're not wrapping the delay in an H.fork? And shouldn't some debouncer be used here:

  1. user clicks the button, which copies the content to the clipboard and starts a debouncer. If the copy doesn't fail, a "Copied to Clipboard" message is displayed. If it does fail, a "Failed to Copy" is displayed.
  2. if user clicks again before debouncer ends, debounder restarts
  3. if debouncer times out, then the text changes back to "Share URL"

Then, the state would be just showConfirmation :: Boolean or perhaps Maybe String where Nothing = "Share URL" and Just s = s where s is either "Failed to Copy" or "Copied to Clipboard". If you initialized this component with a Ref, you could use that to store the reference to the debouncer's ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't debouncing mean something like: we delay writing to clipboard until N milliseconds have passed without another click event? I think I'd still want to write to clipboard immediately, but just keep the notification message showing for some time after. I'm new to Halogen, so there might be a way to leverage debouncing to accomplish that that I'm unaware of; maybe I'd need to setup an event emitter and then debounce some "hide notification message" event?
I've got a prototyped solution using H.fork and storing the ForkId in state here: https://try.purescript.org/?gist=5605231047155a3bf4b834daa03b510e
I'm not sure if there's a simpler way, but that will at least allow showing either a success or failure message as appropriate. I'll plan on going with that unless I find a simpler solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using H.fork (and cancelling previous handleAction) here 6eb73be

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't debouncing mean something like: we delay writing to clipboard until N milliseconds have passed without another click event?

No, we'd write to the clipboard immediately. We would delay when we change from "Copied to Clipboard" back to the original text of "Share Link". If multiple clicks happen, we keep displaying "Copied to Clipboard". Once the debouncer times out because the user hasn't clicked that button for a while, we change back to "Share Link".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the recent changes implement that behavior, though I'm not explicitly using a debounce function.

render :: Int -> H.ComponentHTML Unit () Aff
render n =
HH.li
[ HP.class_ $ HH.ClassName "menu-item no-mobile" ]
[ HH.label
[ HP.id "share_label"
, HP.title "Share URL"
, HE.onClick \_ -> unit
]
[ HH.text (if n > 0 then "✔️ Copied to clipboard" else "Share URL") ]
]

menuRadio
:: forall w
. { name :: String
Expand Down Expand Up @@ -505,13 +531,13 @@ toAnnotation markerType { position, message } =
, text: message
}

withSession :: String -> Aff { sourceFile :: Maybe SourceFile, code :: String }
withSession sessionId = do
state <- H.liftEffect $ tryRetrieveSession sessionId
githubId <- H.liftEffect $ getQueryStringMaybe "github"
withSession :: Aff { sourceFile :: Maybe SourceFile, code :: String }
withSession = do
Comment on lines +546 to +547
Copy link
Contributor Author

@pete-murphy pete-murphy Aug 27, 2022

Choose a reason for hiding this comment

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

Also maybe the withSession name no longer makes sense, now that this doesn't take a sessionId

Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on what this could be renamed to then?

Copy link
Contributor Author

@pete-murphy pete-murphy Sep 4, 2022

Choose a reason for hiding this comment

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

Couldn't think of anything better 😅 so I'll just leave it

state <- H.liftEffect $ getQueryStringMaybe "purs"
githubId <- H.liftEffect $ getQueryStringMaybe "github"
gistId <- H.liftEffect $ getQueryStringMaybe "gist"
code <- case state of
Just { code } -> pure code
code <- case state >>= decompressFromEncodedURIComponent of
Just code -> pure code
Nothing -> do
let
action = oneOf
Expand Down
5 changes: 5 additions & 0 deletions client/src/Try/QueryString.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
export {
compressToEncodedURIComponent,
decompressFromEncodedURIComponent as decompressFromEncodedURIComponent_,
} from 'lz-string';

export function getQueryString() {
return window.location.search;
}
Expand Down
12 changes: 12 additions & 0 deletions client/src/Try/QueryString.purs
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ module Try.QueryString
, getQueryStringMaybe
, setQueryString
, setQueryStrings
, compressToEncodedURIComponent
, decompressFromEncodedURIComponent
) where

import Prelude

import Data.Array as Array
import Data.Maybe (Maybe(..))
import Data.Newtype (wrap)
import Data.Nullable (Nullable, toMaybe)
import Data.String as String
import Data.Tuple (Tuple(..))
import Effect (Effect)
Expand Down Expand Up @@ -58,3 +61,12 @@ setQueryStrings :: Object.Object String -> Effect Unit
setQueryStrings ss = do
params <- getQueryParams
runEffectFn1 setQueryParameters (Object.union ss params)

-- | Compress a string to a URI-encoded string using LZ-based compression algorithm
foreign import compressToEncodedURIComponent :: String -> String

foreign import decompressFromEncodedURIComponent_ :: String -> Nullable String

-- | Decompress a string from a URI-encoded string using LZ-based compression algorithm
decompressFromEncodedURIComponent :: String -> Maybe String
decompressFromEncodedURIComponent = toMaybe <$> decompressFromEncodedURIComponent_
16 changes: 0 additions & 16 deletions client/src/Try/Session.js

This file was deleted.

55 changes: 0 additions & 55 deletions client/src/Try/Session.purs

This file was deleted.