Skip to content

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