Skip to content

Conversation

@talaviram
Copy link

This avoid overlapping / improved iOS device view.

Before this PR:

IMG_4735

After:

IMG_4736

@peitschie peitschie self-requested a review June 2, 2025 06:31
@peitschie
Copy link
Collaborator

peitschie commented Jun 2, 2025

Thanks for taking the time to raise this!

Interesting bug you've encountered here! I wish there was an easier way to workaround this...

The new interface you've added here looks ok, but it has a lot of visual and behaviour differences compared to the version it replaces.

Main things that stick out to me are:

  • The colours, fonts, layout and button shape are all very different from the original dialog
  • This dialog has a fixed height that takes up the majority of the screen, making it far less attractive if there are only one or two devices in the scan that are applicable

Would you have the time to try and align it a little more closely with the UIAlertController it replaces?

@talaviram
Copy link
Author

Ok, I've tried to make it feel more like a dialog for devices.
(mind that as you see also in the current alert the Scanning... is very small).

Anyway, more blurry like iOS and on iOS 15+ I can easily make the view take less space.
For iPad and macOS that was never an issue as it shows as a modal in the center.

Hope this is closer to what you had in mind.

image

@talaviram
Copy link
Author

Sorry for bumping this, but I'd like to use the main branch for another project which I want to PR and this really improves experience on iOS.

Thank you again.

@peitschie
Copy link
Collaborator

I shall endeavour to look at this soon @talaviram

Apologies, time slipped away from me on this.

titleLabel.text = title
}

func setCancelButton(_ title: String?, action: @escaping () -> Void) {

Choose a reason for hiding this comment

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

action is never used here i think

Copy link
Collaborator

Choose a reason for hiding this comment

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

@talaviram any chance you've got some time to double-check this comment?

Copy link
Author

Choose a reason for hiding this comment

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

Hey, sorry. been busy :)

@PAFriedrichMueller thank you for taking the time and discovering the missing call.

I've pushed a commit that can be squashed/fixup (so it'll be easier to review).
I've simply forgot to pass the action closure properly (oops).
Let me know if it's working as expected now.

prior to this, it used Alert view that could easily be cluttered when
scanning without filters in a modern enviornment with > 10 devices.

this now uses UIPopoverPresentationController and UITableView to allow
scrollable list.
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.

3 participants