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
[frontend] Added auto collapse and submenu icon switches to settings #6341
[frontend] Added auto collapse and submenu icon switches to settings #6341
Conversation
482033f
to
6dc29d7
Compare
6dc29d7
to
b7c8b44
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6341 +/- ##
==========================================
- Coverage 67.69% 67.65% -0.05%
==========================================
Files 532 532
Lines 65004 64997 -7
Branches 5445 5433 -12
==========================================
- Hits 44006 43975 -31
- Misses 20998 21022 +24 ☔ View full report in Codecov by Sentry. |
b7c8b44
to
2e680cd
Compare
2e680cd
to
ffa7ff3
Compare
84a7fec
to
5dff342
Compare
ffa7ff3
to
542e139
Compare
Hello, thank you for your contribution ! We love the idea of saving auto collapse and submenu icons as preferences. But there is an issue with the implementation because the Settings page is not a page accessible by all the users (admin page). Below, the implementation we suggest to do so every user can set their preferences:
|
542e139
to
b2123cd
Compare
@lndrtrbn Thanks for the feedback! All your points have been addressed in the latest push. |
b2123cd
to
3d85864
Compare
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 have tested locally and it works fine 👍
I just make some small remarks that should be easily fixed.
I was also questioned myself about this behavior: if several person use the same computer and browser then the state of the left navigation saved in local storage will collide between those users and they will have inconsistencies when login. It could be fixed by using the id of the user in the key for local storage. But I don't know if it is a real issue that need to be tackled, I'll ask info in the team.
EDIT : this local storage collision won't be tackle here
@@ -2673,5 +2673,7 @@ | |||
"Strategic": "Stratégique", | |||
"Link created": "Lien créé", | |||
"Public dashboard": "Tableau de bord public", | |||
"Show left navigation submenu icons": "Afficher les icônes du sous-menu de navigation de gauche", |
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.
"Show left navigation submenu icons": "Afficher les icônes du sous-menu de navigation de gauche", | |
"Show left navigation submenu icons": "Afficher les icônes des sous-menus de la navigation de gauche", |
@@ -2673,5 +2673,7 @@ | |||
"Strategic": "Stratégique", | |||
"Link created": "Lien créé", | |||
"Public dashboard": "Tableau de bord public", | |||
"Show left navigation submenu icons": "Afficher les icônes du sous-menu de navigation de gauche", | |||
"Auto collapse submenus in left navigation": "Sous-menus à réduction automatique dans la navigation de gauche", |
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.
"Auto collapse submenus in left navigation": "Sous-menus à réduction automatique dans la navigation de gauche", | |
"Auto collapse submenus in left navigation": "Fermer automatiquement les sous-menus de la navigation de gauche", |
@@ -220,25 +226,47 @@ const LeftBar = () => { | |||
data: useRef(null), | |||
settings: useRef(null), | |||
}; | |||
const [selectedMenu, setSelectedMenu] = useState(null); | |||
// console.log(localStorage.getItem('selectedMenu')); |
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.
Leftover of debug I guess
menuToggleSubmenuIcons: { | ||
marginRight: 15, | ||
}, |
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.
It seems this style is never used?
style: { | ||
whiteSpace: 'wrap', | ||
lineHeight: 1.2, | ||
padding: '4px 0 4px 10px', |
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.
Is this padding property really useful? It doesn't seem to change much, the less custom css we have, the better
R.assoc('submenu_show_icons', newUser.submenu_show_icons), | ||
R.assoc('submenu_auto_collapse', newUser.submenu_auto_collapse), |
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'm not sure we should add those lines as those properties are not present in the input to create a user. So it will be always undefined.
3d85864
to
44264e9
Compare
Proposed changes
Related issues
Checklist