Skip to content
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

Merged
merged 6 commits into from
Aug 30, 2022
Merged
Show file tree
Hide file tree
Changes from all 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 packages/toolpad-app/next.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ function parseBuidEnvVars(env) {
TOOLPAD_DEMO: env.TOOLPAD_DEMO || '',
TOOLPAD_VERSION: pkgJson.version,
TOOLPAD_CREATE_WITH_DOM: process.env.TOOLPAD_CREATE_WITH_DOM,
TOOLPAD_DEBUG: process.env.TOOLPAD_DEBUG,
};
}

Expand Down
4 changes: 3 additions & 1 deletion packages/toolpad-app/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ export type BuildEnvVars = Record<
| 'TOOLPAD_VERSION'
// Enable input field for seeding a dom in the app creation dialog
// (For testing purposes)
| 'TOOLPAD_CREATE_WITH_DOM',
| 'TOOLPAD_CREATE_WITH_DOM'
// Show debug logs
| 'TOOLPAD_DEBUG',
string
>;

Expand Down
56 changes: 51 additions & 5 deletions packages/toolpad-app/src/toolpad/AppEditor/AppEditorShell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ import {
IconButton,
Stack,
TextField,
Tooltip,
Typography,
} from '@mui/material';
import CloudDoneIcon from '@mui/icons-material/CloudDone';
import * as React from 'react';
import { useForm } from 'react-hook-form';
import { Outlet, useNavigate } from 'react-router-dom';
Expand Down Expand Up @@ -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;
Expand All @@ -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 ? (
Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

@apedroferreira apedroferreira Aug 31, 2022

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

) : (
<CloudDoneIcon
color={hasUnsavedChanges ? 'disabled' : 'success'}
fontSize="medium"
/>
)}
</Box>
</Tooltip>
) : null}
<Typography>{domLoader.unsavedChanges} unsaved change(s).</Typography>
<IconButton
aria-label="Create release"
color="inherit"
Expand Down
26 changes: 25 additions & 1 deletion packages/toolpad-app/src/toolpad/DomLoader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
| {
Expand Down Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The 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"?

Copy link
Member Author

@apedroferreira apedroferreira Aug 31, 2022

Choose a reason for hiding this comment

The 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;
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions packages/toolpad-app/src/utils/logs.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export function debugLog(message: string, color: string): void {
Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Member Author

@apedroferreira apedroferreira Aug 31, 2022

Choose a reason for hiding this comment

The 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.

Copy link
Member

@Janpot Janpot Aug 31, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
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.

if (process.env.TOOLPAD_DEBUG) {
// eslint-disable-next-line no-console
console.log(`%c[DEBUG] ${message}`, `color: ${color}`);
}
}