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

Add fast edit option from preview #1603

Merged
merged 7 commits into from
Feb 1, 2023
Merged

Add fast edit option from preview #1603

merged 7 commits into from
Feb 1, 2023

Conversation

bytasv
Copy link
Contributor

@bytasv bytasv commented Jan 27, 2023

@bytasv bytasv requested a review from a team January 27, 2023 13:44
@bytasv bytasv self-assigned this Jan 27, 2023
@oliviertassinari oliviertassinari requested a deployment to fast-edit - toolpad-db PR #1603 January 27, 2023 13:44 — with Render Abandoned
@oliviertassinari oliviertassinari temporarily deployed to fast-edit - toolpad PR #1603 January 27, 2023 13:45 — with Render Destroyed
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

I left a comment but nothing major and might be a bit opinionated.

Also more opinionated design feedback:

  • I feel like the shadow in the button and the blue color it's using don't match the Alert color too well? Could it be flatter and more consistent in color maybe?
  • Also the font in the button seems a bit small.
  • Another thing I might try, but not sure if it would look better, would be to place the button on the right side or center of the Alert bar. But it's just an idea!

Screenshot 2023-01-30 at 12 54 28

packages/toolpad-app/src/runtime/ToolpadApp.tsx Outdated Show resolved Hide resolved
@prakhargupta1
Copy link
Member

It came up in my discussion with @gerdadesign what if we continue to show the top header bar in the preview mode? We can just replace Preview with Edit and there won't be any need to show 'This is a preview version of the application'.

Would there be any concerns with this approach?

@Janpot
Copy link
Member

Janpot commented Jan 30, 2023

Would there be any concerns with this approach?

The preview header only shows up on preview version of the application. For the edit button this could be fine. But we were talking about adding a header bar that could house the hamburger button that opens the navigation menu when it's hidden on smaller screens.

So the header we talked about during meeting would also need to show up on deployed applications.

@prakhargupta1
Copy link
Member

prakhargupta1 commented Jan 30, 2023

So the header we talked about during meeting would also need to show up on deployed applications.

Yeah, so basically, there are 2 headers just like in this app. But as we discussed, let's skip the inner/app header for now. We'll only deploy an app with a sidebar. #813

We can separately deal with how to handle Toolpad apps on smaller screens.

@Janpot
Copy link
Member

Janpot commented Jan 30, 2023

Ok, I misunderstood. So you want to show this header in the deployed/preview apps as wel:

Screenshot 2023-01-30 at 19 08 07

Works for me. It will need to be hidden as well when hideShell is used, so the app can still be embedded

@prakhargupta1
Copy link
Member

So you want to show this header in the deployed/preview apps as welL

Only for preview, not in the deployed app.

@Janpot
Copy link
Member

Janpot commented Jan 30, 2023

Only for preview, not in the deployed app.

oh ok, then we can't use it to house the hamburger menu for the navigation drawer

@oliviertassinari oliviertassinari requested a deployment to fast-edit - toolpad-db PR #1603 January 31, 2023 13:51 — with Render Abandoned
@bytasv
Copy link
Contributor Author

bytasv commented Jan 31, 2023

@prakhargupta1 @apedroferreira @Janpot I've replaced alert with app header:

CleanShot 2023-01-31 at 15 51 29

@prakhargupta1
Copy link
Member

LGTM!

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

I really like this solution!
Just noticed one more thing, in the preview the logo is vertically stretched for some reason (actually it's the blue font near the logo I think?):

Editor:
Screenshot 2023-01-31 at 16 21 49

Preview:
Screenshot 2023-01-31 at 16 21 56

Doesn't make it look much worse or anything but it's just weird that it's different.

@oliviertassinari oliviertassinari temporarily deployed to fast-edit - toolpad PR #1603 February 1, 2023 10:33 — with Render Destroyed
@bytasv
Copy link
Contributor Author

bytasv commented Feb 1, 2023

@apedroferreira fixed the difference in font

@oliviertassinari oliviertassinari temporarily deployed to fast-edit - toolpad PR #1603 February 1, 2023 15:37 — with Render Destroyed
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Looks great now. Thanks for the changes!

@bytasv bytasv merged commit f615a32 into master Feb 1, 2023
@bytasv bytasv deleted the fast-edit branch February 1, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faster way to edit a deployed/preview app
5 participants