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

Multiple Instances: Polish Tasks #1962

Merged
merged 4 commits into from
Apr 12, 2023
Merged

Conversation

Pezmc
Copy link
Contributor

@Pezmc Pezmc commented Apr 12, 2023

Description

  • Fixes UI of application name when deleting
  • Prevents opening editor also navigating to another page
  • Adds application to devices table

Screenshot 2023-04-12 at 18 16 36

Screenshot 2023-04-12 at 18 16 31

Screenshot 2023-04-12 at 18 16 20

Related Issue(s)

Fixes #1950, part of #1914

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@Pezmc Pezmc requested a review from joepavitt April 12, 2023 09:18
Copy link
Contributor

@joepavitt joepavitt left a comment

Choose a reason for hiding this comment

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

Awaiting tests to run before merging, but all looks good, thanks Pez.

@Steve-Mcl
Copy link
Contributor

Assuming the "Application" and "Instance" column entries are clickable, should we not indicate these are hyperlinks (blue text perhaps)?

At first glance, I have no idea I can click these.

@joepavitt
Copy link
Contributor

Assuming the "Application" and "Instance" column entries are clickable, should we not indicate these are hyperlinks (blue text perhaps)?

Appreciate the feedback Steve, having every application/instance name as blue text would be a little overwhelming with the highlighting of clickable things imo, but I do agree with your point. I don't want to rush in something for tomorrow, so will have a think about alternatives

@joepavitt joepavitt merged commit b550217 into main Apr 12, 2023
@joepavitt joepavitt deleted the feat-1914-polish-multiple-instances branch April 12, 2023 10:22
@Steve-Mcl
Copy link
Contributor

Upon driving latest staging env, I note that it is now a wee bit more difficult to delete an instance,

Prior to these changes, the application-name-to-delete was on its own line and could simply be triple clicked to highlight the full thing.

Now I have to highlight with mouse :( (and waste many valuable milliseconds!)

Before:
chrome_Tdzy5SyMq3

After:
chrome_48CE2Bgkig


its the little things 😄

@Pezmc
Copy link
Contributor Author

Pezmc commented Apr 13, 2023

I think one could argue that it being slightly inconvenient is part of the "are you sure" design, and a "normal user" isn't going to be deleting a bunch of instances at once. Perhaps we could make the text click to copy though.

@Steve-Mcl
Copy link
Contributor

click to copy

Even betterer

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.

Add application to devices API endpoint (as well as instance)
3 participants