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

support multi-platform #4

Merged
merged 9 commits into from
Nov 8, 2021
Merged

support multi-platform #4

merged 9 commits into from
Nov 8, 2021

Conversation

HuixingWong
Copy link
Contributor

Hello, i like your library and want using this on a multi-platform project so i make a small change for support it

@riggaroo
Copy link
Collaborator

riggaroo commented Nov 7, 2021

Hi @HuixingWong, thanks so much for picking this up! Indeed its a request that #2 also is asking for :)

I've run it locally, but I think there is something wrong with the Canvas drawing logic on Desktop, it looks like it maybe doesn't support BlendMode.Multiply on Line 71 of SaturationValueArea, since the black underlying color doesn't come through correctly.

On startup on a phone:
Screenshot_1636267337

On startup on Desktop:
Screenshot 2021-11-07 at 08 42 26

It also seems to be drawing a weird color when you play around with the Hue Bar which I haven't seen on the Android version (looks pretty but its not correct 😄 )

Screenshot 2021-11-07 at 08 43 27

desktop/build.gradle.kts Outdated Show resolved Hide resolved
…el here and 'Modulate' mode no required Android API level 29 and above )
@riggaroo
Copy link
Collaborator

riggaroo commented Nov 7, 2021

Nice, modulate seems to fix that issue 🎉 I'll give it another look through and test everything again, but this is looking really good, thanks so much!

@riggaroo
Copy link
Collaborator

riggaroo commented Nov 7, 2021

I was actually curious about the Blend Mode issue, and have now discovered it does the same thing on API levels lower than 29 😭 I see you changed it to Modulate, I wonder if that works on lower Android API levels. Let me investigate a bit more and see!

api(compose.runtime)
api(compose.foundation)
api(compose.material)
implementation("com.github.ajalt.colormath:colormath:3.1.1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mostly thinking out loud here, but I'm wondering if there isn't a way to only use this dependency only if its the Desktop version, and if that makes sense to do. I think all we need it for is color conversions to and from HSV right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that we can rely on this only on the desktop, but there are a lot of color conversion tools in this library, and since I don't know so much about color things , it might be useful to keep it if you want to expand other functions for this library in the future, What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me give it a bit more thinking and chat with the rest of the GoDaddy Studio Android Team about it tomorrow.

Otherwise, the PR looks good and it is now working for me on Desktop. I'll revert back tomorrow 👍🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright I think we are good to go with this library 💯 Thanks so much for the contribution, merging it now.

@riggaroo riggaroo merged commit c484b04 into godaddy:main Nov 8, 2021
riggaroo pushed a commit that referenced this pull request Feb 17, 2022
support multi-platform
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.

2 participants