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

feat: Use updated minimal NSColorWell style for Groups. #5016

Merged
merged 2 commits into from Feb 27, 2023

Conversation

nevack
Copy link
Member

@nevack nevack commented Feb 23, 2023

Only on macOS Ventura (13.0+), as AppKit changes were introduced here.

Somewhat fixes #5011

Comparison
Before After
Screenshot 2023-02-24 at 01 20 12 Screenshot 2023-02-24 at 01 18 13

@ckerr
Copy link
Member

ckerr commented Feb 23, 2023

@nevack thanks for the before after. 👍

UI LGTM

@ckerr
Copy link
Member

ckerr commented Feb 23, 2023

@nevack needs a whitespace change to make the linter happy:

https://github.com/transmission/transmission/actions/runs/4257356667/jobs/7407395917

@ckerr ckerr added the needs update The PR has needs to be updated by the submitter label Feb 23, 2023
@nevack nevack force-pushed the nevack/updated-nscolorwell-style branch from cad2ed0 to 4113721 Compare February 23, 2023 22:48
@nevack
Copy link
Member Author

nevack commented Feb 23, 2023

@nevack needs a whitespace change to make the linter happy:

transmission/transmission/actions/runs/4257356667/jobs/7407395917

Done, thanks

@nevack
Copy link
Member Author

nevack commented Feb 23, 2023

Oof, we use Xcode 13.x on CI, so no macOS 13 SDK available :(

@ckerr ckerr removed the needs update The PR has needs to be updated by the submitter label Feb 23, 2023
@ckerr
Copy link
Member

ckerr commented Feb 23, 2023

looks like we could bump it to a macOS 12 runner.

Try a global replace of macos-11 with macos-12 in .github/workflows/actions/yml and let's see what happens.

@nevack
Copy link
Member Author

nevack commented Feb 23, 2023

looks like we could bump it to a macOS 12 runner.

Try a global replace of macos-11 with macos-12 in .github/workflows/actions/yml and let's see what happens.

There are some other concerns.
AFAIK @Coeur uses older Xcode locally, so better to provide compatibility layer as we did previously.

@nevack nevack force-pushed the nevack/updated-nscolorwell-style branch from 18ab5ab to fef0245 Compare February 23, 2023 23:17
@nevack nevack requested a review from Coeur February 23, 2023 23:29
@Coeur
Copy link
Collaborator

Coeur commented Feb 23, 2023

Code looks good. Let me check if it builds properly on older Xcode tomorrow.

@Coeur
Copy link
Collaborator

Coeur commented Feb 25, 2023

UI-wise: there is no regression in functionality, as there is a button to access the whole color wheel. So it's good.

@ckerr
Copy link
Member

ckerr commented Feb 25, 2023

To repeat a question from another ticket: is this a bugfix or a new feature?

Asking because the code looks like a fix and the description says "somewhat fixes", but the title describes it as a feature, so I'm wondering if there's some aspect I'm missing.

@ckerr ckerr force-pushed the nevack/updated-nscolorwell-style branch from fef0245 to 04b5c8b Compare February 25, 2023 13:05
@Coeur
Copy link
Collaborator

Coeur commented Feb 25, 2023

It's a UI fix. Doesn't offer extra functionality. Good to merge as patch.

Only on macOS Ventura (13.0+), as AppKit changes were introduced here.

Somewhat fixes transmission#5011

Signed-off-by: Dzmitry Neviadomski <nevack.d@gmail.com>
This APIs are available only with Xcode 14.1 or later.

Signed-off-by: Dzmitry Neviadomski <nevack.d@gmail.com>
@ckerr ckerr force-pushed the nevack/updated-nscolorwell-style branch from 04b5c8b to 631ebf6 Compare February 26, 2023 19:52
@ckerr ckerr merged commit 4430f72 into transmission:main Feb 27, 2023
@nevack nevack deleted the nevack/updated-nscolorwell-style branch February 27, 2023 02:37
@ckerr
Copy link
Member

ckerr commented Mar 2, 2023

Notes: Fixed minor UI bugs, e.g. layout and control alignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Color well of groups is cut at the top
3 participants