-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Settings Advanced Mode #7130
Settings Advanced Mode #7130
Conversation
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.
PR Summary
This pull request implements an Advanced Mode feature for the settings sidebar in the Twenty application. Here's a summary of the key changes:
- Added a new AdvancedSettingsToggle component at the bottom of the settings sidebar
- Implemented a Developers section in the settings navigation, visible when Advanced Mode is enabled
- Created a Recoil atom to manage the Advanced Mode state, persisting it in local storage
- Updated the NavigationDrawer component to accommodate the new Advanced Mode toggle
- Adjusted layout and styling in various components to ensure proper alignment and responsiveness
Key points:
- New AdvancedSettingsToggle component with yellow-themed styling and icon
- Animated Developers section appears when Advanced Mode is enabled
- Recoil state management for Advanced Mode toggle
- Layout adjustments in DefaultLayout and NavigationDrawer for consistency
- Added IconTool to TablerIcons for use in the new feature
10 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
import { localStorageEffect } from '~/utils/recoil-effects'; | ||
|
||
export const isAdvancedModeEnabledState = atom<boolean>({ | ||
key: 'isAdvancedModeEnabledAtom', |
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.
style: Consider using a more descriptive key, e.g., 'isAdvancedModeEnabled'
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.
good job using recoil state!
I think there are two UI issues though
- the tool icon size compared to the line is not the same as on the figma I think
from the PR:
<img width="197" alt="Capture d’écran 2024-09-23 à 17 07 35" src="https://github.com/user-attachments/assets/3f1ba280-1afc-4fd4-81a6-caa37e546f
- there is a glitch when untoggling the advanced mode:
https://github.com/user-attachments/assets/474616a1-b885-4ebf-a8e6-7c6e4056de8c
Hello @ijreilly |
@gitstart-twenty |
@gitstart-twenty there is still a glitch, at expand-time now: 369937339-474616a1-b885-4ebf-a8e6-7c6e4056de8c.movWe are super exacting on the UI aiming at a super smooth platform :) |
`; | ||
|
||
const StyledIconTool = styled(IconTool)` | ||
margin-right: 3px; |
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 usually don't hardcode the values for margins, and usually work in 4x values (4Px, 8px, etc.; sometimes 2px for gaps). Are you sure this respects the figma (I temporarily don't have dev mode access it seems so cannot check)? Or is this an attempt to center things (we should not rely on margin size for that)? Or if it's because it is taking into account a border (in that case, try using box-sizing)
height: { duration: 0.4 }, | ||
}, | ||
}, | ||
exit: { |
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.
could be put in common with initial (exact same values if im not mistaken)
Hello @ ijreilly |
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.
Hello @ijreilly. we pushed the changes 🤝 |
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.
looking good !
Log
|
This PR moved the settings navigation to the left and bottom #7130 Updating the logic to: -remove logic that move the existing -add the setting icon to absolute <img width="264" alt="Capture d’écran 2024-10-07 à 18 04 05" src="https://github.com/user-attachments/assets/b848a5dd-50e9-48c2-bb50-1dcffa9481ac"> <img width="264" alt="Capture d’écran 2024-10-07 à 18 04 11" src="https://github.com/user-attachments/assets/3812929c-dce0-410b-8caa-3ea1210af958">
### Description - We implemented the Advanced Mode state and used this on a section of the settings sidebar - in DefaultLayout.tsx, was updated because of the 64 + 16(container size of IconTool + the margins) ### <https://jam.dev/c/29bcec70-0b7f-4afa-98e6-9755657cf09d> ### Refs twentyhq#6147 Fixes twentyhq#6147 --------- Co-authored-by: gitstart-twenty <[email protected]> Co-authored-by: gitstart-twenty <[email protected]>
This PR moved the settings navigation to the left and bottom twentyhq#7130 Updating the logic to: -remove logic that move the existing -add the setting icon to absolute <img width="264" alt="Capture d’écran 2024-10-07 à 18 04 05" src="https://github.com/user-attachments/assets/b848a5dd-50e9-48c2-bb50-1dcffa9481ac"> <img width="264" alt="Capture d’écran 2024-10-07 à 18 04 11" src="https://github.com/user-attachments/assets/3812929c-dce0-410b-8caa-3ea1210af958">
Description
https://jam.dev/c/29bcec70-0b7f-4afa-98e6-9755657cf09d
Refs
#6147
Fixes #6147