Hide number of changes in UI, add it in debug logging utility #861

apedroferreira · 2022-08-26T16:29:09Z

Closes #781 (fix just for UX, not the number of updates yet)

Show either Saving changes... (saving), You have unsaved changes. (unsaved changes, not saving) or All changes saved! in UI
Create small debug logging infrastructure with a new TOOLPAD_DEBUG environment variable and utils/logs with a method for debug logs. Feel free to suggest changes or alternatives about this.
Log unsaved/saved DOM changes if debugging is active, this should be helpful in fixing any issues with number of updates

Screen.Recording.2022-08-26.at.17.24.44.mov

render · 2022-08-26T16:29:14Z

Your Render PR Server URL is https://toolpad-pr-861.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cc4f9m1a6gdmo9nc4i2g.

bharatkashyap · 2022-08-27T12:08:45Z

Show either Saving changes... (saving), You have unsaved changes. (unsaved changes, not saving) or All changes saved! in UI

I would suggest the "All changes saved" to be transient (disappear after a few second delay), benchmarking based on how Google Docs does it:

Screen.Recording.2022-08-27.at.5.37.09.PM.mov

Janpot · 2022-08-29T07:57:18Z

I would suggest the "All changes saved" to be transient (disappear after a few second delay), benchmarking based on how Google Docs does it:

Yes, taking it a bit further even, to reduce the amount of space it takes in the UI and avoid layout shifting, perhaps it makes sense to just use an icon + tooltip for this?
We could use color to indicated "all saved"/"has unsaved changes" and a spinner to indicate "saving"?

apedroferreira · 2022-08-29T15:05:36Z

I would suggest the "All changes saved" to be transient (disappear after a few second delay), benchmarking based on how Google Docs does it:

Good idea! I'll add something like that.

Yes, taking it a bit further even, to reduce the amount of space it takes in the UI and avoid layout shifting, perhaps it makes sense to just use an icon + tooltip for this?
We could use color to indicated "all saved"/"has unsaved changes" and a spinner to indicate "saving"?

On one hand this makes the information we're conveying less clear, but this information is also a bit secondary, so I think this would make sense. I'll try something like this too.

render · 2022-08-29T21:20:56Z

Your Render PR Server URL is https://toolpad-pr-861.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cc6irdhgp3jian8oe6m0.

apedroferreira · 2022-08-30T16:30:21Z

With icon and tooltip (disappears after 4.5 seconds without changing, forgot to record that):

Screen.Recording.2022-08-30.at.17.29.39.mov

bharatkashyap · 2022-08-30T16:37:01Z

packages/toolpad-app/src/toolpad/AppEditor/AppEditorShell.tsx

+            <Tooltip title={getSaveStateMessage(isSaving, hasUnsavedChanges)}>
+              <Box display="flex" flexDirection="row" alignItems="center">
+                {isSaving ? (
+                  <CircularProgress size={16} color="inherit" sx={{ mr: 1 }} />


I feel that we don't need the CircularProgress icon for the isSaving state: it is not discernable to me in the video as a separate icon; perhaps we can add a "pulsating" animation to the icon in the isSaving state

Anyway, that could be a follow-up improvement! This looks great to me overall as a first step

it is not discernable to me in the video as a separate icon

keep in mind this video is made on a locally running app, irl it will often take longer to complete the request

yeah it's showing as long as saving is true, looks like in dev it's a pretty short time.
lmk if you think we should show the loader when there are unsaved changes even though the saving flag is false

Janpot

Looking good 👍

we could also use the CloudDone icon for this

apedroferreira · 2022-08-30T16:57:17Z

Looking good 👍

we could also use the CloudDone icon for this

ok i'll make a quick switch before merging, it's more techy i guess :D

Janpot

Had a few more comments

Janpot · 2022-08-31T08:56:24Z

packages/toolpad-app/src/utils/logs.ts

@@ -0,0 +1,6 @@
+export function debugLog(message: string, color: string): void {


@apedroferreira This looks like a function that is only there to circumvent // eslint-disable-next-line no-console. Can't we just use console.log, so that we don't get tempted to start logging everything?

i made this a separate function to abstract away the environment variable check and the color in the logs. but sure, we can write it every time too. i'll change it.

Perhaps we can leave out the color and the env check?

Not sure we need the env check if we use logging sparingly

Not sure what the color brings, but if we use it, we should probably think about the semantic meaning of it

i was thinking maybe we didn't want to show certain logs to end users so i added this environment variable, we could just show them to everyone though.
about the color, i could have made the function only take certain values such as success, warning, error, info, but i'm fine with not having colors for now especially if we don't have cases for all these types of logs yet.

Janpot · 2022-08-31T08:58:37Z

packages/toolpad-app/src/toolpad/AppEditor/AppEditorShell.tsx

-            <Box display="flex" flexDirection="row" alignItems="center">
-              <CircularProgress size={16} color="inherit" sx={{ mr: 1 }} />
-            </Box>
+          {isSaveStateVisible ? (


I don't think we should hide the icon. It will cause layout shift and prevent us from adding more things in the header

ok, i can use something like visibility: hidden

What's the reason to hide it exactly?

oh i see, i was thinking the save state wasn't that helpful after a while so it could stay hidden - that's also what Bharat proposed and i implemented with the timeout (it disappears after a few seconds). but we can keep the green cloud there as long as it's saved too.

Janpot · 2022-08-31T08:59:51Z

packages/toolpad-app/src/toolpad/DomLoader.tsx

+      'orange',
+    );
+  } else if (previousUnsavedChanges > 0) {
+    debugLog('All changes saved!', 'green');


To reduce the amount of logs, perhaps we can just do one message "Saved X changes"?

i can change it, just has a bit less visibility into changes that don't all happen together and the save state. maybe we don't need all that though.

Hide number of changes in UI, add it in debug logging utility

29a223f

apedroferreira self-assigned this Aug 26, 2022

oliviertassinari requested a deployment to hide-number-of-changes - toolpad-db PR #861 August 26, 2022 16:29 — with Render Abandoned

Merge remote-tracking branch 'origin/master' into hide-number-of-changes

23cdae2

Use timeout for save state message

6144e51

oliviertassinari requested a deployment to hide-number-of-changes - toolpad-db PR #861 August 29, 2022 21:20 — with Render Abandoned

Use icon and tooltip for save state

14fb041

apedroferreira requested review from bharatkashyap and Janpot August 30, 2022 16:30

bharatkashyap reviewed Aug 30, 2022

View reviewed changes

bharatkashyap approved these changes Aug 30, 2022

View reviewed changes

Janpot approved these changes Aug 30, 2022

View reviewed changes

apedroferreira added 2 commits August 30, 2022 18:00

Use cloud icon

3fafd01

Merge remote-tracking branch 'origin/master' into hide-number-of-changes

54337aa

apedroferreira merged commit 43a7a01 into master Aug 30, 2022

apedroferreira deleted the hide-number-of-changes branch August 30, 2022 17:18

Janpot reviewed Aug 31, 2022

View reviewed changes

apedroferreira mentioned this pull request Sep 1, 2022

Editor and save state tweaks #879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide number of changes in UI, add it in debug logging utility #861

Hide number of changes in UI, add it in debug logging utility #861

apedroferreira commented Aug 26, 2022 •

edited

Loading

render bot commented Aug 26, 2022

bharatkashyap commented Aug 27, 2022 •

edited

Loading

Janpot commented Aug 29, 2022 •

edited

Loading

apedroferreira commented Aug 29, 2022 •

edited

Loading

render bot commented Aug 29, 2022

apedroferreira commented Aug 30, 2022 •

edited

Loading

bharatkashyap Aug 30, 2022

bharatkashyap Aug 30, 2022

Janpot Aug 30, 2022

apedroferreira Aug 30, 2022

Janpot left a comment

apedroferreira commented Aug 30, 2022

Janpot left a comment

Janpot Aug 31, 2022

apedroferreira Aug 31, 2022 •

edited

Loading

Janpot Aug 31, 2022 •

edited

Loading

apedroferreira Aug 31, 2022

Janpot Aug 31, 2022

apedroferreira Aug 31, 2022

Janpot Aug 31, 2022

apedroferreira Aug 31, 2022 •

edited

Loading

Janpot Aug 31, 2022

apedroferreira Aug 31, 2022 •

edited

Loading

		@@ -0,0 +1,6 @@
		export function debugLog(message: string, color: string): void {

Hide number of changes in UI, add it in debug logging utility #861

Hide number of changes in UI, add it in debug logging utility #861

Conversation

apedroferreira commented Aug 26, 2022 • edited Loading

render bot commented Aug 26, 2022

bharatkashyap commented Aug 27, 2022 • edited Loading

Janpot commented Aug 29, 2022 • edited Loading

apedroferreira commented Aug 29, 2022 • edited Loading

render bot commented Aug 29, 2022

apedroferreira commented Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot left a comment

Choose a reason for hiding this comment

apedroferreira commented Aug 30, 2022

Janpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Janpot Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

apedroferreira commented Aug 26, 2022 •

edited

Loading

bharatkashyap commented Aug 27, 2022 •

edited

Loading

Janpot commented Aug 29, 2022 •

edited

Loading

apedroferreira commented Aug 29, 2022 •

edited

Loading

apedroferreira commented Aug 30, 2022 •

edited

Loading

apedroferreira Aug 31, 2022 •

edited

Loading

Janpot Aug 31, 2022 •

edited

Loading

apedroferreira Aug 31, 2022 •

edited

Loading

apedroferreira Aug 31, 2022 •

edited

Loading