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

feat(console): wing console application reset capability #6665

Merged
merged 34 commits into from
Jun 14, 2024

Conversation

polamoros
Copy link
Contributor

@polamoros polamoros commented Jun 10, 2024

Resolves #6124

Copy link

Thanks for opening this pull request! 🎉
Please consult the contributing guidelines for details on how to contribute to this project.
If you need any assistance, don't hesitate to ping the relevant owner over Discord.

Topic Owner
Wing SDK and utility APIs @chriscbr
Wing Console @ainvoner, @skyrpex, @polamoros
JSON, structs, primitives and collections @hasanaburayyan
Platforms and plugins @hasanaburayyan
Frontend resources (website, react, etc) @tsuf239
Language design @chriscbr
VSCode extension and language server @markmcculloh
Compiler architecture, inflights, lifting @yoav-steinberg
Wing Testing Framework @tsuf239
Wing CLI @markmcculloh
Build system, dev environment, releases @markmcculloh
Library Ecosystem @chriscbr
Documentation @hasanaburayyan
SDK test suite @tsuf239
Examples @hasanaburayyan
Wing Playground @eladcon

@monadabot
Copy link
Contributor

monadabot commented Jun 10, 2024

Console preview environment is available at https://wing-console-pr-6665.fly.dev 🚀

Last Updated (UTC) 2024-06-14 08:50

@polamoros
Copy link
Contributor Author

polamoros commented Jun 10, 2024

I am adding a header with a 'Restart simulator' button on the left and moving the Discord button from the footer to the right side of the header.

Thoughts?

@skyrpex @ainvoner @eladb

image image

eladb
eladb previously requested changes Jun 10, 2024
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

@polamoros I am not a fan of the new header.

I think the "RESET" button should be on the top right of the map view:

Screenshot 2024-06-10 at 16 20 31

And it should just say "Reset".

When clicking on it, there should be a message box:

Are you sure you want to reset all state and restart the application?

YES/NO

The Discord button can stay on the footer AFAIK for now.

apps/wing-console/console/server/src/router/app.ts Outdated Show resolved Hide resolved
@polamoros
Copy link
Contributor Author

@polamoros I am not a fan of the new header.

I think the "RESET" button should be on the top right of the map view:

Screenshot 2024-06-10 at 16 20 31

And it should just say "Reset".

When clicking on it, there should be a message box:

Are you sure you want to reset all state and restart the application?

YES/NO

The Discord button can stay on the footer AFAIK for now.

I don't like the header either. Regarding moving the reset button to the map controls, I don't think it's the correct solution. We are restarting the application, not just the map.
I would prefer moving it to the footer or adding a menu on the right side of the header (if we decide to keep the header).

Here's an example
image

Regarding the Discord button, @ainvoner wants to place it in a more visible location.

@skyrpex
Copy link
Contributor

skyrpex commented Jun 10, 2024

@polamoros I am not a fan of the new header.

I think the "RESET" button should be on the top right of the map view:

Screenshot 2024-06-10 at 16 20 31

And it should just say "Reset".

When clicking on it, there should be a message box:

Are you sure you want to reset all state and restart the application?

YES/NO

The Discord button can stay on the footer AFAIK for now.

It doesn't make sense to me to have such a functionality inside the map view: the reset action affects the whole application, not just the map view. Also, it may signal to the users that it only resets the view (same as the [] button on the right side).

Even if a new header is almost empty, it makes sense to place the reset button there IMO.

@polamoros
Copy link
Contributor Author

polamoros commented Jun 11, 2024

What about moving it to the left side of the footer?

I think it makes more sense.

image image

@eladb @skyrpex @ainvoner

@skyrpex
Copy link
Contributor

skyrpex commented Jun 11, 2024

We could also merge the functionality of the reset button and the Status label. They are similar.

@polamoros polamoros requested a review from eladb June 11, 2024 09:27
@skyrpex
Copy link
Contributor

skyrpex commented Jun 11, 2024

It's strange that the Status label isn't updated when the app is restarting 🤔

@skyrpex
Copy link
Contributor

skyrpex commented Jun 11, 2024

I think the restart button should be also disabled when the simulator is loading (e.g. after editing the wing file).

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

LGTM

@polamoros polamoros requested a review from eladb June 14, 2024 06:54
polamoros and others added 5 commits June 14, 2024 09:49
Co-authored-by: Cristian Pallarés <cristianp@wing.cloud>
Co-authored-by: Cristian Pallarés <cristianp@wing.cloud>
Co-authored-by: Cristian Pallarés <cristianp@wing.cloud>
Co-authored-by: Cristian Pallarés <cristianp@wing.cloud>
@polamoros polamoros requested a review from skyrpex June 14, 2024 07:53
@skyrpex skyrpex dismissed eladb’s stale review June 14, 2024 08:39

Already said LGTM

Copy link
Contributor

mergify bot commented Jun 14, 2024

Thanks for contributing, @polamoros! This PR will now be added to the merge queue, or immediately merged if 6124-wing-console-application-reset-capability is up-to-date with main and the queue is empty.

Copy link
Contributor

mergify bot commented Jun 14, 2024

Thanks for contributing, @polamoros! This PR will now be added to the merge queue, or immediately merged if 6124-wing-console-application-reset-capability is up-to-date with main and the queue is empty.

@mergify mergify bot merged commit 06d3a02 into main Jun 14, 2024
14 checks passed
@mergify mergify bot deleted the 6124-wing-console-application-reset-capability branch June 14, 2024 08:53
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.74.60.

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.

Wing Console application “reset” capability
4 participants