[WIP] Use extra React state to track temporary Zarr URL

[andy-sweet]

This is one way to have the text input field reflect the value being currently edited, though it feels oh so wrong.

Maybe a better approach is to load the track manager and set it on the enter key or blur/lose-focus event?

Closes #46

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
points-web-viewer	✅ Ready (Inspect)	Visit Preview	Mar 11, 2024 10:50pm

[andy-sweet]

We probably shouldn't have a visible label and a placeholder unless they contain different information. I slightly prefer the placeholder because it saves space, but no strong preference.

[aganders3]

I'm not sure if there is a design principle we can refer to here. The name "placeholder" to me implies it should be some example text, rather than a label or instruction. Otherwise I agree I would lean toward using the placeholder and removing the label to save space.

[andy-sweet]

Can't find any recommendations from Material: https://m3.material.io/components/text-fields/overview

Looks like the placeholder isn't recommended there - instead a label underneath should be used to explain what this is.

I've seen a placeholder used for instructions or for an example.

I'll just leave it alone in this PR though and we can poke design later to see what their recommendations area.

[aganders3]

I looked in the mui docs and found defaultValue. I think it may just solve this for us?

diff --git a/src/scene.tsx b/src/scene.tsx
index 57e02e0..28e2811 100644
--- a/src/scene.tsx
+++ b/src/scene.tsx
@@ -22,7 +22,6 @@ export default function Scene(props: SceneProps) {
 
     const [trackManager, setTrackManager] = useState<TrackManager>();
     const [dataUrl, setDataUrl] = useState(DEFAULT_ZARR_URL);
-    const [tempDataUrl, setTempDataUrl] = useState(DEFAULT_ZARR_URL.toString());
     const [numTimes, setNumTimes] = useState(0);
     const [curTime, setCurTime] = useState(0);
     const [autoRotate, setAutoRotate] = useState(false);
@@ -175,19 +174,15 @@ export default function Scene(props: SceneProps) {
                         label="Zarr group track data URL"
                         hideLabel={true}
                         placeholder="Zarr group track data URL"
-                        value={tempDataUrl}
-                        onChange={(e) => setTempDataUrl(e.target.value)}
-                        onKeyUp={(e) => {
-                            if (e.key === "Enter") setDataUrl(new URL(tempDataUrl));
-                        }}
-                        onBlur={() => setDataUrl(new URL(tempDataUrl))}
+                        onChange={(e) => setDataUrl(new URL(e.target.value))}
                         fullWidth={true}
+                        defaultValue={DEFAULT_ZARR_URL.toString()}
                         intent={trackManager ? "default" : "error"}
                     />
                     <InputSlider
                         id="time-frame-slider"

[andy-sweet]

I looked in the mui docs and found defaultValue. I think it may just solve this for us?

diff --git a/src/scene.tsx b/src/scene.tsx
index 57e02e0..28e2811 100644
--- a/src/scene.tsx
+++ b/src/scene.tsx
@@ -22,7 +22,6 @@ export default function Scene(props: SceneProps) {
 
     const [trackManager, setTrackManager] = useState<TrackManager>();
     const [dataUrl, setDataUrl] = useState(DEFAULT_ZARR_URL);
-    const [tempDataUrl, setTempDataUrl] = useState(DEFAULT_ZARR_URL.toString());
     const [numTimes, setNumTimes] = useState(0);
     const [curTime, setCurTime] = useState(0);
     const [autoRotate, setAutoRotate] = useState(false);
@@ -175,19 +174,15 @@ export default function Scene(props: SceneProps) {
                         label="Zarr group track data URL"
                         hideLabel={true}
                         placeholder="Zarr group track data URL"
-                        value={tempDataUrl}
-                        onChange={(e) => setTempDataUrl(e.target.value)}
-                        onKeyUp={(e) => {
-                            if (e.key === "Enter") setDataUrl(new URL(tempDataUrl));
-                        }}
-                        onBlur={() => setDataUrl(new URL(tempDataUrl))}
+                        onChange={(e) => setDataUrl(new URL(e.target.value))}
                         fullWidth={true}
+                        defaultValue={DEFAULT_ZARR_URL.toString()}
                         intent={trackManager ? "default" : "error"}
                     />
                     <InputSlider
                         id="time-frame-slider"

Good find, but this just behaves the same as main for me - i.e. every character change to the input text field calls the onChange callback and thus a re-render and all the associated effects with the change to dataUrl.

[aganders3]

Hmm, maybe we're thinking of different problems in that case. I was thinking updates/re-render with dataUrl changes were desired behavior.

What I was trying to report in #46 is (on main) that I can't highlight the URL and delete it. I assume it gets re-populated when value gets set to the current URL. Likewise you can't highlight the URL and start typing. Instead you have to delete character-by-character, and even then eventually it stops deleting when is no longer a valid URL. Instead you have to have a fully-formed URL to paste in or replace parts of the existing URL piece-by-piece.

[andy-sweet]

Agreed there are two different problems here.

I'm down to use defaultValue to fix the issue that you can't easily change the URL at all (other than for pasting, which is a more important usage).

And then think about how we actually want to track every change or have some way to submit that (e.g. using enter).

As this PR mostly does the latter, I'll just close it and reopen a different one for the simple fix.