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: Implement custom primary color #106

Merged
merged 19 commits into from
Mar 28, 2024
Merged

Conversation

Biowulf21
Copy link
Contributor

@Biowulf21 Biowulf21 commented Mar 24, 2024

Hi @sadespresso, this is the feature implementation for #105 .

This is my first time submitting a PR for a FOSS repo, so I do apologize in advance if my code is not up to FOSS standard. I'd like to work on this project on my spare time, as I see a lot of potential in this application 😄 .

@sadespresso
Copy link
Collaborator

Hello @Biowulf21, thank you for taking your time to implement this feature.

It would be more appreciated if we discuss before jumping to implementations. As this is a very young project, I have so many things in mind, and trying my best to list them down in the issues.

Moving forward with your PR, we have few problems.

  • First of all, colornames package seem to support only English, but Flow plans to support multiple languages. So it's best if we define the names locally, or find a library that supports multiple languages. Not having names is fine too.
  • As for the color picker, I was thinking to handpick number of colors and offer them, and they would have different perceived brightness in light/dark mode. This makes localization more easy, and allows us to create different app icons for each color in iOS.

@sadespresso
Copy link
Collaborator

This, for example..

image

Copy link
Collaborator

@sadespresso sadespresso left a comment

Choose a reason for hiding this comment

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

I will have this in Flow if you make following changes:

  • Remove colornames package, and not use color names for now or use a localized color name provider
  • (MAYBE) not require restart after changing theme color. You can write sth in Flow (the main app widget) and have it transcend the changes. Nice to have this, but not necessary.
  • Provide a video demonstrating the implemented feature

pubspec.yaml Outdated Show resolved Hide resolved
assets/l10n/en_US.json Outdated Show resolved Hide resolved
@sadespresso
Copy link
Collaborator

Again, thank you for the PR!

Let me know if you want to proceed with the suggestions and review

@Biowulf21
Copy link
Contributor Author

Hello, @sadespresso. I'll implement these changes as soon as I get home. I'm out of the house at the moment.

As for communicating features moving forward, I'd like to connect with you on discord if that's alright with you.

Thanks.

@sadespresso
Copy link
Collaborator

sadespresso commented Mar 24, 2024

:D :D

We have a discord server, and you can find me, or my account in there!

@Biowulf21
Copy link
Contributor Author

Biowulf21 commented Mar 28, 2024

Hello @sadespresso, can you please point me into the right direction as to which color picker we'll be using? I can refactor my current implementation if you already have a package in mind.

As for the other changes you requested, I've already gone through them. Please let me know if you have any questions/suggestions on the implementation.

Lastly, here is the video demo that you requested for the feature:

Thanks.

@sadespresso
Copy link
Collaborator

Hello @Biowulf21

Thank you so much for the work! I really appreciate it.

I will check it out once I have time, should be soon.

@sadespresso
Copy link
Collaborator

LGTM.

Thank you!


In the future, I plan to give option to use dynamic colors. It's the new Android 13 thing I believe...

Also, in the future, we'd have much more simpler color selector.

@sadespresso sadespresso merged commit 05e1e6e into flow-mn:main Mar 28, 2024
1 check passed
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