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

Ironsides - Move implementation closer to Design's vision #3284

Merged
merged 7 commits into from
Jun 27, 2024

Conversation

timkur
Copy link
Contributor

@timkur timkur commented Jun 22, 2024

Summary of the pull request

This updates the code to get closer to what the latest comps from Design look like.

Before
OldDetached
OldHoriztonal
OldVirt
OldExpanded

After

Screenshot 2024-06-25 101051
Screenshot 2024-06-25 101107
Screenshot 2024-06-25 101122
Screenshot 2024-06-25 101415

References and relevant issues

Detailed description of the pull request / Additional comments

Of the changes:

A label on the Process buttons
When attached to a process, the "pick a process button" no longer shows in the title bar
The process name is shown in the bar now instead of the PID
The Expanded view will show the Insights and WER reports page even if you're not attached to a process
The Expanded view Settings menu item has turned into an icon in the bottom right hand corner
The Expanded view's Process name Title has been removed (since the process name is in the bar)

Validation steps performed

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
  • Telemetry compliance tasks completed for added/updated events

@andreww-msft
Copy link
Contributor

Hmm... I feel this needs a little more design/UX input...

  1. Why labels on some buttons but not others, that's just weird. How would the user understand the logic of which items get labels and which ones don't?
  2. Also, with labels alongside the buttons instead of underneath, they take up a lot of space. I routinely have 4-6 external tools - how's that going to work? Labels for all of them, only some of them? Regardless, we'll run out of space before I get to 4 tools...
  3. We explicitly moved the Settings button to the bottom left to make it consistent with Dev Home core - why move it back again?
  4. Insights right now is PI's least useful page - why does this get promoted to the bar? Is this just a hangover from months ago when we thought we'd get to do more insights much earlier?
  5. Why are we still showing app cpu usage in the top bar when it's also in the bottom bar? Why are we showing app cpu usage in the top bar at all? Perhaps also just a hangover from early prototypes?

@timkur
Copy link
Contributor Author

timkur commented Jun 24, 2024

Hmm... I feel this needs a little more design/UX input...

  1. Why labels on some buttons but not others, that's just weird. How would the user understand the logic of which items get labels and which ones don't?
  2. Also, with labels alongside the buttons instead of underneath, they take up a lot of space. I routinely have 4-6 external tools - how's that going to work? Labels for all of them, only some of them? Regardless, we'll run out of space before I get to 4 tools...

It looks like the only buttons in the bar that don't have labels are the external tools, and it's part of the bootstrapping to get folks going. As part of the upcoming UX study, Tyler is going to look to see if these labels are necessary, if they should disappear after a period of time... I think you had the suggestion of making this configurable, etc.

  1. We explicitly moved the Settings button to the bottom left to make it consistent with Dev Home core - why move it back again?

I'll defer to @TWhiteDesigns for that. One thing that this (probably) inadvertently does is open up "contextual settings" based on the page the user is currently looking at. I don't think that works when it's on the left with the list of other items to display.

  1. Insights right now is PI's least useful page - why does this get promoted to the bar? Is this just a hangover from months ago when we thought we'd get to do more insights much earlier?

I'm happy to remove the button for now.

  1. Why are we still showing app cpu usage in the top bar when it's also in the bottom bar? Why are we showing app cpu usage in the top bar at all? Perhaps also just a hangover from early prototypes?

The latest design comp has, once the window is expanded, the system-wide resources disappearing from the top right corner and moving into the status bar. I left this alone for now as I wanted to talk to Tyler a bit more about UX elements moving around based on the window size. Additionally, we've talked about making this all configurable, so not sure if that means we have a certain set of Perf Counters available for configuration on the bar, and another set configurable for the status bar.

@andreww-msft
Copy link
Contributor

Re labels, in addition to the external tools, there's the External Tools button itself. Also, I don't get why the custom chrome buttons don't have labels if the main bar buttons do. IMHO we should not introduce any labels, and instead make this user-configurable (either all buttons have labels, or none). Plus, labels should be under the button not alongside. Moving some of the functional buttons to the chrome type makes that decision more difficult, unfortunately: more input on this aspect from @TWhiteDesigns, please.

Re Settings button, I'm not following - why does the left vs right position make any difference to whether we introduce contextual settings?

Re resource usage, moving the system-wide items from top to bottom when expanded makes sense - are you saying that the per-app cpu item will be removed from the top? or from the bottom? Right now it's in both places. Looking ahead, yes these should be user-configurable, and imho there should be a "resource usage panel" where the user gets to choose whatever resource items she wants, either per-app or system-wide or both. I filed #3296 for this.

@krschau krschau added this to the Dev Home 0.16 milestone Jun 24, 2024
@timkur
Copy link
Contributor Author

timkur commented Jun 24, 2024

Re labels, in addition to the external tools, there's the External Tools button itself. Also, I don't get why the custom chrome buttons don't have labels if the main bar buttons do. IMHO we should not introduce any labels, and instead make this user-configurable (either all buttons have labels, or none). Plus, labels should be under the button not alongside. Moving some of the functional buttons to the chrome type makes that decision more difficult, unfortunately: more input on this aspect from @TWhiteDesigns, please.

I see examples of labels being applied to some buttons but not others, presumably to help direct users if the icon itself isn't super straightforward?

image

image

image

So I don't think the design is super crazy. :)

Re Settings button, I'm not following - why does the left vs right position make any difference to whether we introduce contextual settings?

Technically we could do it either way. However, the way I see it, everything on the left hand side in the "navigation" area brings up its own thing. I would expect if I clicked a settings button on the left hand side to bring up the main settings page. Items on the right hand side, especially once the clipboard monitor tool moves, looks like it could be part of the per-page content. That's just where my eyes go.

But regardless, I'm just following design's lead.

Re resource usage, moving the system-wide items from top to bottom when expanded makes sense - are you saying that the per-app cpu item will be removed from the top? or from the bottom? Right now it's in both places. Looking ahead, yes these should be user-configurable, and imho there should be a "resource usage panel" where the user gets to choose whatever resource items she wants, either per-app or system-wide or both. I filed #3296 for this.

The current design doesn't remove the per-CPU usage from the top. Presumably users could want some type of resource monitoring in a minimal bar.

@andreww-msft
Copy link
Contributor

Tim and I chatted, I'm happy with the changes, this is an evolving space, and this PR definitely moves things in the right direction.

@TWhiteDesigns
Copy link

Hi team, catching up on the thread here. As Andrew mentioned, this is an evolving space, and these moves are a step in the process. Will follow up directly tomorrow with specifics/details on the top-level questions as much of these moves relate to topics we're looking to gain insight on via the upcoming UR study.

@timkur timkur requested a review from krschau June 26, 2024 17:24
@@ -277,6 +293,9 @@ private void TargetApp_PropertyChanged(object? sender, PropertyChangedEventArgs
ApplicationPid = process.Id;
ApplicationName = process.ProcessName;
}

// Conversely, the process chooser is only visible if we're not attached to a process
IsProcessChooserVisible = process is null;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: how about we assign IsProcessChooserVisible before IsAppBarVisible and then set IsAppBarVisible = !IsProcessChooserVisible, keeping it consistent with the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently I do the opposite... I'm not sure what you mean by keeping it consistent with the constructor. Can you elaborate?

    IsProcessChooserVisible = process is null;
    IsAppBarVisible = !IsProcessChooserVisible;

I did update the code, since immediately thereafter I was checking for "if (process is not null)" and it was more clear to say "if (IsAppBarVisible)"...

@timkur timkur merged commit 997a176 into main Jun 27, 2024
4 checks passed
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.

5 participants