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

App card enhancements #591

Merged
merged 12 commits into from
Jun 24, 2022
Merged

App card enhancements #591

merged 12 commits into from
Jun 24, 2022

Conversation

bharatkashyap
Copy link
Member

  • Fixes App Card: Enhancements/Fixes #534

  • I have followed (at least) the PR section of the contributing guide.

  • Adds readable time durations and moves app title to the center of the card (inside a <CardContent>)

  • Extracts <EditableText> out as a separate component (using a TextField and Typography component together since hard to get a disabled TextField to wrap in case of long names)

  • Handles errors when trying to create an app with an existing name

Screenshot 2022-06-22 at 9 04 38 PM

@render
Copy link

render bot commented Jun 22, 2022

@oliviertassinari oliviertassinari temporarily deployed to app-card-enhance - toolpad-db PR #591 June 22, 2022 19:14 — with Render Destroyed
</DialogActions>
</DialogForm>
</Dialog>
<Snackbar
Copy link
Member

@Janpot Janpot Jun 23, 2022

Choose a reason for hiding this comment

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

We will only use snackbars when errors happen without context. In this case we can show loading state and error state inline in the dialog, no snackbar needed. The error should be available in createAppMutation.error

subheader={
<Typography variant="body2" color="text.secondary">
{app ? `Edited: ${app.editedAt.toLocaleString('short')}` : <Skeleton />}
{app ? `Edited ${getReadableDuration(app.editedAt)}` : <Skeleton />}
Copy link
Member

Choose a reason for hiding this comment

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

We still need to make the absolute time accessible for the users as well. Perhaps using a <time> tag. Or with a Tooltip.

readableDuration = '1 hour ago';
} else if (delta < day) {
readableDuration = `${Math.floor(delta / hour)} hours ago`;
} else if (delta < day * 2) {
Copy link
Member

Choose a reason for hiding this comment

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

For the specific use case of the apps overview, I think we can stop at "x hours ago". there will be too many duplicates otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would apps created a few days or weeks ago say in that case?

Copy link
Member

Choose a reason for hiding this comment

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

Just the date?

Copy link
Member Author

Choose a reason for hiding this comment

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

Something like this?
Screenshot 2022-06-23 at 2 39 20 PM

/>
{showAppNameErrorAlert ? (
Copy link
Member

@Janpot Janpot Jun 23, 2022

Choose a reason for hiding this comment

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

Would it make sense to use the TextField error state and helperText for this?

} catch (err) {
setShowNameErrorAlert(true);
} catch (error) {
// Silently catch
Copy link
Member

@Janpot Janpot Jun 23, 2022

Choose a reason for hiding this comment

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

One way we may be able to avoid this is by running

window.location.href = `/_toolpad/app/${app.id}/editor`;

in the onSuccess handler of useMutation and use createAppMutation.mutate([name]); without await instead

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Looking good 👌

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 24, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 24, 2022
@bharatkashyap bharatkashyap merged commit d819021 into master Jun 24, 2022
@bharatkashyap bharatkashyap deleted the app-card-enhance branch June 24, 2022 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App Card: Enhancements/Fixes
3 participants