Admin settings dashboard and modal addition for user components #1310
Conversation
… with the units creation flow
…l needs to be changed to cards model, like units.
There was a problem hiding this comment.
Thanks to @hazeltonbw for doing this work.
I have done a high level review of this PR. Given my comments I want to see how they are resolved before doing a complete review as it may lead to nontrivial change. I'm happy to discuss any comments and hear your thoughts.
Here are a couple of high level comments that did not fit anywhere or I could not put in comments in the file:
- The test build on GitHub is failing because the new translation files do no conform to what OED needs. I suspect they may go away but if not then this must be addressed.
- I'm unclear on the new en.ts, fr.ts and es.ts translation files. They overlap the data.ts ones. I recall a team meeting where they were discussed and I thought we resolved not to separate out the translations (at least not now). Could you help me understand and let me know if my recollection is off?
| @@ -0,0 +1,54 @@ | |||
| /* This Source Code Form is subject to the terms of the Mozilla Public | |||
There was a problem hiding this comment.
While I think this change to a sidebar is interesting, I have a couple of thoughts:
- This is only done on this one page and I wonder if OED is going to do this should it be part of all the pages that are appropriate such as meters. However, on meters it is a modal so formatting and layout would need to be considered. Given this, I think the project should discuss whether or not to do this.
- If OED is going to do this generally then we should consider how. For example, react-pro-sidebar is a very fancy one (maybe too much) but packages should be analyzed before we do this via our own styling.
I'm open to ideas but right now I'm inclined to remove this and create something new that addresses this one idea.
Just a few overview comments for the record:
- The items are not alphabetical as are menus in OED.
- Users are being removed from this page.
- Might be nice to have an all option to see everything.
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| import * as React from 'react'; |
There was a problem hiding this comment.
I want to understand how this relates to PR #1306 that created cards/modal for users. First, if it is the same basic work then this PR must be held until that work is review/approved in the separate PR. Second, the plan was to remove user from the admin panel (now site settings) so not sure if/where this belongs.
|
|
||
| interface PreferencesProps { | ||
| selectedPreference: string; | ||
| } | ||
|
|
||
| // TODO: Add warning for invalid data |
|
|
||
| interface PreferencesProps { | ||
| selectedPreference: string; | ||
| } | ||
|
|
||
| // TODO: Add warning for invalid data | ||
| /** |
There was a problem hiding this comment.
ESLint warning on missing param in JSDoc.
| @@ -0,0 +1,76 @@ | |||
| /* eslint-disable no-mixed-spaces-and-tabs */ | |||
There was a problem hiding this comment.
Same question from other file on how relates to other PR. Also applies to UsersDetailsComponent.
src/client/app/translations/data.ts
Outdated
| @@ -503,34 +504,34 @@ const LocaleTranslationData = { | |||
| }, | |||
| "fr": { | |||
| "3D": "3D", | |||
| "400": "400 Bad Request\u{26A1}", | |||
| "400": "400 Bad Request", | |||
There was a problem hiding this comment.
I am unclear on why you removed the special symbol at the end of untranslated strings and seem not to have done for all. Please see the PR that created them and then let me know if you think they should still be removed.
src/client/app/translations/data.ts
Outdated
| "return.dashboard": "Return To Dashboard", | ||
| "role": "Rôle", | ||
| "save.all": "Sauver tous", | ||
| "save.map.edits": "Enregistrer les modifications de la carte", |
There was a problem hiding this comment.
You seem to have translated strings that are not directly involved in this PR. Is that correct or am I missing something? If so then: First, should this be a separate PR? Second, since you only did a few, is this a language you are very good at so the translations will be of high quality?
|
This PR has been superseded by PR #1316 and will now be closed. |
Description
In this PR the main focus was to streamline the admin panel to make it more user friendly and future proof for additional features.
There were also changes to the components that allow admins to manage users. These changes were made to stay consistent with the design choices from other pages which utilizes modals to create, edit, and delete users.
Fixes: #890
Type of change
Checklist
Limitations