-
-
Notifications
You must be signed in to change notification settings - Fork 55
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/color testing #88
Conversation
|
||
const ColorHarmonies = () => { | ||
const [color, setColor] = useState<string>("#61D2C3"); | ||
|
||
const [colorDeb] = useDebouncedValue(color, 300); |
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.
Removed unnecessary rendering on the /color-util tab.
The elements were initially added on the color-utils since they were hidden by default. However, since they are no longer there, it is no longer necessary to debounce them on the /harmonies tab.
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.
fair. Can you also remove the unused imports and fix any other eslint errors in the same module? I saw some unused vars and imports in prev PR
src/Features/colors/hooks.ts
Outdated
import { useCallback, useEffect } from "react"; | ||
import { getRandomColor } from "./utilities"; | ||
|
||
export const useColorRandomizer = (setColor: (color: string) => void) => { |
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.
if this is working it's fine, but ideally if you are creating a custom hook, it's a good idea to have states local to the hook. you can return those states from the hook, react can optimize this.
for example
const [color] = useColorRandomizer();
and then have state inside the hook
const useColorRandomizer() {
const [color, setColor] = useState();
// .... rest
return [color, setColor]
}
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.
LGTM
Once your changes are finalized, let me know, I will merge and release after that |
@Sparkenstein Just pushed the cleanup, will be starting on the UI changes right now in a new branch. Mantine + scss 👑 |
great changes 🙌 thanks. |
Yeah for sure, thinking about setting up some more components like the MonacoWrapper and using a full typeof- prop. Just making the ui changes feel a bit more professional and clean. |
Note: will add the explanations later;
Since I'll create some custom UI blocks like popovers/ tooltips for that.
Cleanup of color-utils
Keeping it more focused on the actual convertion.