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 icons to device labels #3650

Merged
merged 7 commits into from Nov 8, 2021
Merged

Add icons to device labels #3650

merged 7 commits into from Nov 8, 2021

Conversation

guidezpl
Copy link
Contributor

@guidezpl guidezpl commented Nov 3, 2021

Screen Shot 2021-11-03 at 18 26 34

Screen Shot 2021-11-03 at 22 55 25

Closes #3647

@guidezpl
Copy link
Contributor Author

guidezpl commented Nov 3, 2021

@DanTup I don't have permission to request a review, please review :)

@DanTup DanTup added this to the v3.29.0 milestone Nov 4, 2021
@DanTup DanTup added in flutter Relates to running Flutter apps is enhancement labels Nov 4, 2021
Copy link
Member

@DanTup DanTup left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! :-)

@DanTup
Copy link
Member

DanTup commented Nov 4, 2021

Oh, maybe we should add a mobile icon for "Create Android Emulator"?

@guidezpl
Copy link
Contributor Author

guidezpl commented Nov 4, 2021

It might be more clear to only show icons for available devices and keep the commands as is

Before open
Screen Shot 2021-11-04 at 14 12 24
After open
Screen Shot 2021-11-04 at 14 13 22

@DanTup
Copy link
Member

DanTup commented Nov 4, 2021

Yeah, I think you're right - although the alignment looks a bit weird. Is there any blank/spacer we could use so the text all lines up? If not, then I'll merge as-is. Thanks!

@guidezpl
Copy link
Contributor Author

guidezpl commented Nov 5, 2021

There isn't, but just in case, here's what it looks like with icons:
Screen Shot 2021-11-05 at 03 57 57. WDYT?

@DanTup
Copy link
Member

DanTup commented Nov 5, 2021

I think that looks ok, but I wouldn't say miles better so I'll leave it to you.

It might be better if we could have a separator like shown here:

image

But I can't find the code doing that - I'm not sure if it's VS Code doing it from some special API for notebooks, or if we can do this ourselves 🤔

@guidezpl
Copy link
Contributor Author

guidezpl commented Nov 5, 2021

It's coming next release apparently! I filed #3655. I'll go with icons

@DanTup
Copy link
Member

DanTup commented Nov 5, 2021

Great!

There's a failing test on the PR:

  2) device_manager
       overrides real emulators with custom definitions:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '$(play) Start My emulator override'
- 'Start My emulator override'
      + expected - actual

      -$(play) Start My emulator override
      +Start My emulator override

To run, select Flutter or Flutter LSP tests in the debug pane (you can also change the test to it.only( to run just that test) - although the fix is probably fairly clear :-)

Thanks!

@DanTup
Copy link
Member

DanTup commented Nov 8, 2021

Thanks!

@DanTup DanTup merged commit 5f49df5 into Dart-Code:master Nov 8, 2021
@guidezpl guidezpl deleted the device-icons branch November 8, 2021 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in flutter Relates to running Flutter apps is enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show icons against devices based on their type
2 participants