-
-
Notifications
You must be signed in to change notification settings - Fork 287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hide number of changes in UI, add it in debug logging utility #861
Changes from 4 commits
29a223f
23cdae2
6144e51
14fb041
3fafd01
54337aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,10 @@ import { | |
IconButton, | ||
Stack, | ||
TextField, | ||
Tooltip, | ||
Typography, | ||
} from '@mui/material'; | ||
import CheckCircleIcon from '@mui/icons-material/CheckCircle'; | ||
import * as React from 'react'; | ||
import { useForm } from 'react-hook-form'; | ||
import { Outlet, useNavigate } from 'react-router-dom'; | ||
|
@@ -102,6 +104,16 @@ function CreateReleaseDialog({ appId, open, onClose }: CreateReleaseDialogProps) | |
); | ||
} | ||
|
||
function getSaveStateMessage(isSaving: boolean, hasUnsavedChanges: boolean): string { | ||
if (isSaving) { | ||
return 'Saving changes...'; | ||
} | ||
if (hasUnsavedChanges) { | ||
return 'You have unsaved changes.'; | ||
} | ||
return 'All changes saved!'; | ||
} | ||
|
||
export interface ToolpadAppShellProps { | ||
appId: string; | ||
actions?: React.ReactNode; | ||
|
@@ -111,18 +123,52 @@ export default function AppEditorShell({ appId, ...props }: ToolpadAppShellProps | |
const domLoader = useDomLoader(); | ||
|
||
const [createReleaseDialogOpen, setCreateReleaseDialogOpen] = React.useState(false); | ||
const [isSaveStateVisible, setIsSaveStateVisible] = React.useState(false); | ||
|
||
const hasUnsavedChanges = domLoader.unsavedChanges > 0; | ||
const isSaving = domLoader.saving; | ||
|
||
React.useEffect(() => { | ||
let timeout: NodeJS.Timeout | null = null; | ||
|
||
if (!hasUnsavedChanges) { | ||
timeout = setTimeout(() => { | ||
setIsSaveStateVisible(false); | ||
|
||
if (timeout) { | ||
clearTimeout(timeout); | ||
} | ||
}, 4500); | ||
} else { | ||
setIsSaveStateVisible(true); | ||
} | ||
|
||
return () => { | ||
if (timeout) { | ||
clearTimeout(timeout); | ||
} | ||
}; | ||
}, [hasUnsavedChanges]); | ||
|
||
return ( | ||
<ToolpadAppShell | ||
appId={appId} | ||
actions={ | ||
<Stack direction="row" gap={1} alignItems="center"> | ||
{domLoader.saving ? ( | ||
<Box display="flex" flexDirection="row" alignItems="center"> | ||
<CircularProgress size={16} color="inherit" sx={{ mr: 1 }} /> | ||
</Box> | ||
{isSaveStateVisible ? ( | ||
<Tooltip title={getSaveStateMessage(isSaving, hasUnsavedChanges)}> | ||
<Box display="flex" flexDirection="row" alignItems="center"> | ||
{isSaving ? ( | ||
<CircularProgress size={16} color="inherit" sx={{ mr: 1 }} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel that we don't need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, that could be a follow-up improvement! This looks great to me overall as a first step There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
keep in mind this video is made on a locally running app, irl it will often take longer to complete the request There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it's showing as long as |
||
) : ( | ||
<CheckCircleIcon | ||
color={hasUnsavedChanges ? 'disabled' : 'success'} | ||
fontSize="medium" | ||
/> | ||
)} | ||
</Box> | ||
</Tooltip> | ||
) : null} | ||
<Typography>{domLoader.unsavedChanges} unsaved change(s).</Typography> | ||
<IconButton | ||
aria-label="Create release" | ||
color="inherit" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import client from '../api'; | |
import useShortcut from '../utils/useShortcut'; | ||
import useDebouncedHandler from '../utils/useDebouncedHandler'; | ||
import { createProvidedContext } from '../utils/react'; | ||
import { debugLog } from '../utils/logs'; | ||
|
||
export type DomAction = | ||
| { | ||
|
@@ -250,6 +251,27 @@ export function useDomApi(): DomApi { | |
return React.useContext(DomApiContext); | ||
} | ||
|
||
let previousUnsavedChanges = 0; | ||
function logUnsavedChanges(unsavedChanges: number) { | ||
const hasUnsavedChanges = unsavedChanges >= 1; | ||
const newChanges = unsavedChanges - previousUnsavedChanges; | ||
|
||
const pluralizeChanges = (changesCount: number) => `change${changesCount !== 1 ? 's' : ''}`; | ||
|
||
if (hasUnsavedChanges) { | ||
debugLog( | ||
`+ ${newChanges} ${pluralizeChanges( | ||
newChanges, | ||
)}. ${unsavedChanges} unsaved ${pluralizeChanges(unsavedChanges)}.`, | ||
'orange', | ||
); | ||
} else if (previousUnsavedChanges > 0) { | ||
debugLog('All changes saved!', 'green'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To reduce the amount of logs, perhaps we can just do one message "Saved X changes"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
previousUnsavedChanges = unsavedChanges; | ||
} | ||
|
||
export interface DomContextProps { | ||
appId: string; | ||
children?: React.ReactNode; | ||
|
@@ -294,13 +316,15 @@ export default function DomProvider({ appId, children }: DomContextProps) { | |
}, [state.dom, debouncedhandleSave]); | ||
|
||
React.useEffect(() => { | ||
logUnsavedChanges(state.unsavedChanges); | ||
|
||
if (state.unsavedChanges <= 0) { | ||
return () => {}; | ||
} | ||
|
||
const onBeforeUnload = (event: BeforeUnloadEvent) => { | ||
event.preventDefault(); | ||
event.returnValue = `You have ${state.unsavedChanges} unsaved change(s), are you sure you want to navigate away?`; | ||
event.returnValue = `You have unsaved changes. Are you sure you want to navigate away?`; | ||
}; | ||
|
||
window.addEventListener('beforeunload', onBeforeUnload); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
export function debugLog(message: string, color: string): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @apedroferreira This looks like a function that is only there to circumvent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we can leave out the color and the env check?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if (process.env.TOOLPAD_DEBUG) { | ||
// eslint-disable-next-line no-console | ||
console.log(`%c[DEBUG] ${message}`, `color: ${color}`); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should hide the icon. It will cause layout shift and prevent us from adding more things in the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i can use something like
visibility: hidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason to hide it exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.