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

Improve device list UX #1

Merged
merged 3 commits into from
Jun 5, 2021
Merged

Conversation

MrSkwiggs
Copy link
Contributor

@MrSkwiggs MrSkwiggs commented Jun 3, 2021

First contribution 🙌
First let me thank you for this, I had a cheap chinese led strip and always wondered whether or not I could control from my phone. Running your code for the first time, noticed I also had a Triones showing up and... it just worked right away 💪 Amazing!

Now, I've been playing around a bit and wanted to improve the experience when selecting a peripheral, so here goes 👇

This commit does 3 things:

  • It sorts the peripherals list by their name (and puts any nameless peripherals last)
  • Improves peripheral selection by making the content shape bigger
  • Reduces the opacity of unknown peripherals so that named ones stand out more

Let me know if this suits you :)

@artemnovichkov
Copy link
Owner

Hi, @MrSkwiggs! Thank you so much for the feedback! It's a real pleasure that ColorStripe is useful not only for me 😅. I've checked the PR, DevicesView looks better. I'm wondering is it possible to sort peripherals in DevicesViewModel? In my opinion, it's a better place for this logic, but not sure how it will work with @published wrapper 🤔

@MrSkwiggs
Copy link
Contributor Author

@artemnovichkov I'd love to, but I think that could be part of a larger ViewModel refactor overall.

Right now, it's impossible to write any previews because they all rely on CBPeripheral instances directly (and those can't be instantiated).
I'd suggest using Models for the views as simple structs, taking only what you need from from CBPeripheral (i.e uuid & name) so you can easily mock them, and still map back & forth between the 2.

I have a few ideas on this which I'll play around with a bit, and if that's alright with you I'd tackle that there?

@MrSkwiggs
Copy link
Contributor Author

Also went ahead and slightly edited how the Mode selection cell looks

@artemnovichkov
Copy link
Owner

Thanks for the mode improvement! About models — yeah, I thought about it, especially in preview context. I suggest merging this PR as is and trying to play with models in another one if you don't mind. Is it ok?

@MrSkwiggs
Copy link
Contributor Author

Yeah definitely :) I already have started a separate branch on my fork for that

@artemnovichkov artemnovichkov merged commit dc8029f into artemnovichkov:main Jun 5, 2021
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.

None yet

2 participants