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: customize colors for types #69

Merged
merged 8 commits into from
Dec 17, 2023
Merged

Conversation

flamintune
Copy link

Resolves #47

@MichaelYuhe MichaelYuhe self-requested a review December 12, 2023 14:58
@MichaelYuhe MichaelYuhe added the enhancement New feature or request label Dec 12, 2023
src/popup.tsx Outdated
flexShrink: 0,
}}
></div>
<svg
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think using svg directly in the tsx file is a good idea. Can u try use the svg as an image instead?

src/popup.tsx Outdated
colorsRef.current[idx] = node;
}}
className={`bg-${colors[idx]}-500 w-4 h-4`}
style={{
Copy link
Owner

Choose a reason for hiding this comment

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

className and style seems to be duplicated

src/popup.tsx Outdated
value={colorOption}
onChange={(e) => {
setColor(e.target.value);
console.log(e.target.value);
Copy link
Owner

Choose a reason for hiding this comment

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

remove console.log :D

src/popup.tsx Outdated
type="radio"
name="color"
className={`bg-${colorOption}-500 w-4 h-4 rounded-full`}
style={{
Copy link
Owner

Choose a reason for hiding this comment

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

use className or style

Copy link
Author

Choose a reason for hiding this comment

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

ok,I'll use inline styles. Initially, I used className, but it had no effect, so I switched to style. I also noticed that Chrome's tab groups support 'grey' by default, but Tailwind CSS uses the American spelling 'gray'.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe you can modify the tailwind.config.js

src/popup.tsx Outdated
width="16"
height="16"
onClick={(e) => {
let nextIdx =
Copy link
Owner

Choose a reason for hiding this comment

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

const maybe better

src/popup.tsx Outdated
DEFAULT_COLOR.length;
const newColors = [...colors];
newColors[idx] = DEFAULT_COLOR[nextIdx];
colorsRef.current[idx].animate(
Copy link
Owner

Choose a reason for hiding this comment

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

it seems that you are using refs only for transition for colors? You can just add transition-colors in tailwind to do so. If there is any other reason using refs pls let me know~

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you're right. I was planning to utilize refs to create a translation animation with transform. The sole advantage of using refs might be the convenience it offers when altering the animation later on. Should I delete it...

src/utils.ts Outdated
@@ -31,6 +31,17 @@ export const DEFAULT_GROUP = [
"Utilities",
];

export const DEFAULT_COLOR = [
Copy link
Owner

Choose a reason for hiding this comment

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

Since it's a fixed and unchanging choice for colors, maybe we can use enum for better types

@MichaelYuhe
Copy link
Owner

BTW, since it's a breaking change in popup(which some other users might don't like it, you can check my latest main branch, there is a feature flags tab in options page, you can decide whether to show the colors picker in popup

@flamintune
Copy link
Author

BTW, since it's a breaking change in popup(which some other users might don't like it, you can check my latest main branch, there is a feature flags tab in options page, you can decide whether to show the colors picker in popup

I have a question. If user have created serveral types without customizing colors, when he click this switch to enable customize colors, The color data for displaying types is sourced from where? And when he creates serveral types without customizing colors, do we need to generate a random color that is different from the previous type's color to save?

@MichaelYuhe
Copy link
Owner

BTW, since it's a breaking change in popup(which some other users might don't like it, you can check my latest main branch, there is a feature flags tab in options page, you can decide whether to show the colors picker in popup

I have a question. If user have created serveral types without customizing colors, when he click this switch to enable customize colors, The color data for displaying types is sourced from where? And when he creates serveral types without customizing colors, do we need to generate a random color that is different from the previous type's color to save?

By default we will use a random color unless user has specified a color for this type

@MichaelYuhe
Copy link
Owner

Sorry for the late merge, can you help me to solve the conflicts?

@flamintune
Copy link
Author

It's okay, maybe I forgot to remind you. As a newbie, I really appreciate you letting me participate in the development of this feature and taking the time to review my low quality code and give feedback. If you did it yourself, you would be able to complete this feature very quickly. 😰😰

@MichaelYuhe
Copy link
Owner

It's okay, maybe I forgot to remind you. As a newbie, I really appreciate you letting me participate in the development of this feature and taking the time to review my low quality code and give feedback. If you did it yourself, you would be able to complete this feature very quickly. 😰😰

Hey I really appreciate your work! Without your help I can't bring it to a next level myself!

@MichaelYuhe MichaelYuhe merged commit 49366b1 into MichaelYuhe:main Dec 17, 2023
1 check passed
@lawvs lawvs mentioned this pull request Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Customize colors for types
2 participants