-
Notifications
You must be signed in to change notification settings - Fork 183
Battlegrounds UX clarity #1376
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
base: master
Are you sure you want to change the base?
Battlegrounds UX clarity #1376
Conversation
|
This looks so much better! I can't give constructive feedback on the added code, but the visuals are much closer to HDT. @Humeian I think one more pass on a few of the sizes, and then we should merge this ASAP: I've created a split view for the different elements (HDT is the right one and HSTracker the left one, respectively): Based on that, here's some visual feedback:
|
|
I am curious why I don't see this difference with the existing code. I am worried about the duplication of UIs with this solution as I'd not have two different ways to handle the same UI. The choice is to either figure out what's wrong with the AppKit version or make HST require 10.15 and above, even though Hearthstone still has a minimum requirement of 10.14. Also, there is duplicate code for the Color extension as there is one already that pretty much does the same thing. I will take a closer look at this tonight but I'd rather not have duplicate UIs |
| @available(macOS 10.15, *) | ||
| // Extend SwiftUI color to be able to accept hex string that avgPlacementColor is generating | ||
| // from the placement value. Implementation from: https://blog.eidinger.info/from-hex-to-color-and-back-in-swiftui | ||
| extension Color { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code seems to be duplicate of the code in HSTracker/Core/Extensions/Color.swift. A convenience constructor already exists to use a NSColor and crease a Color object, so this seems like duplicate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't see that, the convenience constructor to convert from NSColor to Color requires macOS 12.0 and would add and extra conversion step, so instead I've abstracted the logic from taking hex into RBGA and used that single function for converting hex to NSColor & Color
|
I looked at the code and in general I don't have a problem except for having duplicate code. So, I would vote for picking one or the other as the way to go. So, either we figure out why AppKit is not working on other systems or we raise the minimum HST version to 10.15 which I assume Hearthstone will do next time they update Unity |
4dc9649 to
13ab848
Compare
|
Let's just hold on this until we can chat more on Discord with fmoraes74 about how we want to proceed with SwiftUI/10.15 vs continuing to support 10.14. |
d653ee6 to
aa3f672
Compare
c04662e to
15ecafb
Compare



Cleans up Battlegrounds Hero & Trinket picking overlays to use the same new SwiftUi view:


Old:
New:


Additionally adds some text bolding to the battlegrounds session headers for increased clarity