fix: issue #4249 #4322
fix: issue #4249 #4322
Conversation
maxceem
left a comment
maxceem
left a comment
There was a problem hiding this comment.
@yoution it works good, but there are some small improvements I'd like to make.
-
We have to wait until the user credentials are loaded, to make sure that when we show the form we already know is user uses SSO login or no. See how we check if we still loading setting or no https://github.com/appirio-tech/connect-app/blob/dev/src/routes/settings/routes/system/containers/SystemSettingsContainer.jsx#L15.
-
There are some small code improvements to make. Please, kindly check comments below.
src/routes/settings/routes/system/containers/SystemSettingsContainer.jsx
Show resolved
Hide resolved
| </div>: null} | ||
|
|
||
| <div className="form"> | ||
| {!usingSsoService ? <div className="form"> |
There was a problem hiding this comment.
this could be simplified as {!usingSsoService && <div className="form">. No need for ternary operator .. ? .. : null.
| </div> | ||
|
|
||
| <div styleName="section-heading"> | ||
| {!usingSsoService ? |
There was a problem hiding this comment.
this could be simplified as {!usingSsoService &&. No need for ternary operator .. ? .. : null.
| disabled={usingSsoService? true: systemSettings.disabled === true} | ||
| /> | ||
|
|
||
| {usingSsoService ? <div styleName="error-message"> |
There was a problem hiding this comment.
this could be simplified as {usingSsoService && <div styleName="error-message">. No need for ternary operator .. ? .. : null.
| checkEmailAvailability={checkEmailAvailability} | ||
| onSubmit={(email) => changeEmail(email)} | ||
| {...systemSettings} | ||
| disabled={usingSsoService? true: systemSettings.disabled === true} |
There was a problem hiding this comment.
To keep things clear, I suggest passing usingSsoService to the ChangeEmailForm component and use it inside. Because we have various reasons when we disable form. Also, this condition is a bit hard to ready.
| isEmail: 'Provide a correct email' | ||
| }} | ||
| disabled={isEmailChanging || !hasPermission(PERMISSIONS.UPDATE_USER_EMAIL)} | ||
| disabled={disabled ||isEmailChanging || !hasPermission(PERMISSIONS.UPDATE_USER_EMAIL)} |
There was a problem hiding this comment.
As I suggest in another comment, let's not pass disabled here, but pass usingSsoService directly here, so we can understand more clearly why we disable this field.
2 participants
No description provided.