-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/s 10 user settings #19
Conversation
* feat: add new schedule table * chore: refactor cells renderers * chore: refactor renderers
…view feat: add switcher schedule view mode
…Hope/rsschool-api into feature/S-10-user-settings
|
||
const SettingsTagColor: React.FC = () => { | ||
const { Panel } = Collapse; | ||
const defaultTagColor = JSON.stringify({ default: DEFAULT_COLOR }); |
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.
According to the source code of useLocalStorage
, there is no need to serialize/deserialize data. It comes out of the box: https://github.com/streamich/react-use/blob/master/src/useLocalStorage.ts
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.
done, removed unnecessary things
borderColor: getTagColor(item.name, storedTagColors), | ||
color: getTagColor(item.name, storedTagColors), | ||
backgroundColor: `${getTagColor(item.name, storedTagColors)}10`, | ||
}} |
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.
Let's move these 4 lines of the code to it's own function and call getTagColor
only once
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.
Made a new function. It returns an object with styles borderColor, color, backgroundColor
, it can also accept any other css styles
colors={pickerColors} | ||
triangle="hide" | ||
width={'138px'} | ||
onChange={(e) => setTagColor(e, item.name, setStoredTagColors, storedTagColors)} |
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.
Let's consider using useCallback
for onChange handler
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.
); | ||
|
||
return ( | ||
<Collapse expandIcon={() => <BgColorsOutlined style={{ fontSize: '20px', color: '#08c' }} />}> |
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.
WDYT about moving () => <BgColorsOutlined style={{ fontSize: '20px', color: '#08c' }} />
to it's own component?
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.
made a new component TagColorIcon
. It contains all the magick :)
|
||
|
||
const UserSettings: React.FC = () => { | ||
const [isVisible, setIsVisible] = useState(false); |
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.
let's replace with isOpen
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.
and it's done
@@ -0,0 +1,53 @@ | |||
import React from 'react'; |
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.
I think TagColor
file name will be a better option. It will avoid duplications and shorten import paths at least.
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.
and it's done too
} | ||
|
||
export function getTagColor(tagName: string, storedTagColors: string | undefined) { | ||
const parsedTagColors = JSON.parse(<string>(storedTagColors)) || {}; |
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.
We can remove JSON.parse and JSON.stringify here too. Also let's move getter and setter to TagSettings file.
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 things. Why should we move getter and setter to TagColor
?
export function setTagColor(e: IColorState, tagName: string, localStorageHook: (value: string) => void, storedTagColors: string | undefined) { | ||
const parsedTagColors = JSON.parse(<string>storedTagColors) || {}; | ||
parsedTagColors[tagName] = e.hex; | ||
localStorageHook(JSON.stringify(parsedTagColors)); |
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.
Can be just: localStorageHook({ ... storedTagColors, [tagName]: e.hex })
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.
You're right! Done
* feat: add new schedule table (#13) * feat: add new schedule table * chore: refactor cells renderers * chore: refactor renderers * feat: add switcher schedule view mode * feat: add user settings * chore: refactor code - exclude switch * chore: refactor code * chore: refactor code - add uppercase * fix: implement correct localstorage hook * feat: add userSettings button on schedule page * feat: add user settings * refactor: change serialize/deserialize data * refactor: rename files and data * refactor: change tag color styles structure * refactor: rename vars for better readability * refactor: apply memoization to setTagColor * refactor: change structure of TagIcon * refactor: replace settings button, tweak button styles, change storedTagColors type Co-authored-by: notSAINT <60464016+not-SAINT@users.noreply.github.com> Co-authored-by: Aleksandr Sharikov <notsaint.mail@gmail.com>
* feat: add new schedule table (#13) * feat: add new schedule table * chore: refactor cells renderers * chore: refactor renderers * feat: add switcher schedule view mode * feat: add user settings * chore: refactor code - exclude switch * chore: refactor code * chore: refactor code - add uppercase * fix: implement correct localstorage hook * feat: add userSettings button on schedule page * feat: add user settings * refactor: change serialize/deserialize data * refactor: rename files and data * refactor: change tag color styles structure * refactor: rename vars for better readability * refactor: apply memoization to setTagColor * refactor: change structure of TagIcon * refactor: replace settings button, tweak button styles, change storedTagColors type Co-authored-by: notSAINT <60464016+not-SAINT@users.noreply.github.com> Co-authored-by: Aleksandr Sharikov <notsaint.mail@gmail.com>
* feat: add new schedule table (#13) * feat: add new schedule table * chore: refactor cells renderers * chore: refactor renderers * feat: add switcher schedule view mode * feat: add user settings * chore: refactor code - exclude switch * chore: refactor code * chore: refactor code - add uppercase * fix: implement correct localstorage hook * feat: add userSettings button on schedule page * feat: add user settings * refactor: change serialize/deserialize data * refactor: rename files and data * refactor: change tag color styles structure * refactor: rename vars for better readability * refactor: apply memoization to setTagColor * refactor: change structure of TagIcon * refactor: replace settings button, tweak button styles, change storedTagColors type Co-authored-by: notSAINT <60464016+not-SAINT@users.noreply.github.com> Co-authored-by: Aleksandr Sharikov <notsaint.mail@gmail.com>
add user settings component