-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fixed : User settings shows uuids instead of names as sync attributes #968 #1091
Conversation
src/adminApp/user.js
Outdated
content.forEach(val => newValMap.set(val.id, val.name)) | ||
); | ||
setValueMap(newValMap); | ||
}, []); |
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.
- Since you are depending on a props value, you should declare it in the dependencies - https://react.dev/reference/react/useEffect#examples-dependencies
- valueMap is a generic name. Add something more meaningful. eg: conceptSyncAttributeValues
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.
Not sure of the purpose of the isMounted flag. How does it help?
Why do you catch an error and ignore it?
|
@
Catch statement should be ignored only if you want to suppress the error. If not, then you should not do this at all. |
…ame and removed unnecessary imports
Notes :
Fixes #968
Presupposition :
For user, it become easy if we shows concpet value in sync attribute.
Tech Task :
Dev Test :
User with sync attribute:
User without sync attribute:
Other Fixes:
As we can see there is no space between two skeleton of sync attribute. So I added margin
Result :