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

Move app settings to a modal and add explanations #1914

Merged
merged 4 commits into from
May 21, 2023
Merged

Conversation

jonatanklosko
Copy link
Member

image image

@github-actions
Copy link

github-actions bot commented May 20, 2023

Uffizzi Preview deployment-25950 was deleted.

@josevalim
Copy link
Contributor

This is looking great, I just want to bikeshed on the panel a bit:

Screenshot 2023-05-20 at 10 12 19
  • Is "App" the best name? Maybe "Deploys" is better?

  • I also think that "Deployments" should be "Latest version" or "Current version"?

  • Also, I think "App sessions" could have its own header, like "Deployments" and maybe call it "Running sessions" or "App sessions" or just "Sessions"?

Thoughts?

Copy link
Contributor

@josevalim josevalim 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. I pushed a bunch of nitpicks

@jonatanklosko
Copy link
Member Author

Is "App" the best name? Maybe "Deploys" is better?

We call the feature "apps" in general and we have "Apps" in the home sidebar, so I think "App" gives a clearer indication of what this sidebar is related to.

I also think that "Deployments" should be "Latest version" or "Current version"?

Sounds good, "Current version" works.

Also, I think "App sessions" could have its own header, like "Deployments" and maybe call it "Running sessions" or "App sessions" or just "Sessions"?

A separate header works for me, the wording should be consistent with the apps page:

image

It could be "sessions", but being explicit doesn't hurt and this makes a clearer distinction between regular sessions and app sessions.

@jonatanklosko
Copy link
Member Author

image

@josevalim
Copy link
Contributor

So let's go with these headers: "Application", "Latest deploy" (or "Current deploy") and "App sessions". :)

@jonatanklosko
Copy link
Member Author

@josevalim we say "app" everywhere, so for consistency it's better than "application", no?

@jonatanklosko
Copy link
Member Author

Current deploy

Deploy is a verb, not sure if that mattes though :p

@josevalim
Copy link
Contributor

Alright so:

"App", "Latest deployment" (or "Current deployment") and "Running sessions"? I don't want to say "App sessions" because App is implied?

@jonatanklosko
Copy link
Member Author

Sounds good!

jonatanklosko and others added 2 commits May 20, 2023 11:28
Co-authored-by: José Valim <jose.valim@dashbit.co>
Copy link
Contributor

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

LGTM after the comments have been addressed. :)

Co-authored-by: José Valim <jose.valim@dashbit.co>
@jonatanklosko jonatanklosko merged commit d1ba07e into main May 21, 2023
6 checks passed
@jonatanklosko jonatanklosko deleted the jk-app-modal branch May 21, 2023 19:21
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.

None yet

2 participants