Editing a location/user/vehicle profile could be neater

[LamVu1]

For issue #34

Changed ClickableTextField and update function for location and vehicle detail.

[LamVu1]

@duckbytes Wanted to check if my work is correct so far. Haven't finished the user profile, I have some questions regarding it.

[codecov-commenter]

Codecov Report

Merging #52 (7d05a35) into master (6119e96) will decrease coverage by 0.18%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   46.58%   46.39%   -0.19%     
==========================================
  Files         263      264       +1     
  Lines        8679     8721      +42     
  Branches     2210     2206       -4     
==========================================
+ Hits         4043     4046       +3     
- Misses       4183     4231      +48     
+ Partials      453      444       -9     

Impacted Files	Coverage Δ
src/scenes/LocationDetail/LocationDetail.js	0.00% <0.00%> (ø)
...cenes/LocationDetail/components/LocationProfile.js	0.00% <0.00%> (ø)
src/scenes/LocationsList.js	0.00% <0.00%> (ø)
src/scenes/UserDetail/UserDetail.js	0.00% <0.00%> (ø)
src/scenes/UserDetail/components/UserProfile.js	0.00% <0.00%> (ø)
src/scenes/VehicleDetail/VehicleDetail.js	0.00% <0.00%> (ø)
.../scenes/VehicleDetail/components/VehicleProfile.js	0.00% <0.00%> (ø)
src/redux/RootSagas.js	100.00% <0.00%> (ø)
src/redux/WhoamiObserverSaga.js
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6119e96...7d05a35. Read the comment docs.

[LamVu1]

@duckbytes In src/scenes/UserDetail/components/UserProfile.js; we have a useRef named toChange to keep track of the values before the user clicks save. If I'm using ClickableTextField then we won't need that. Also for the UserRolesAndSelector, the onSelectRole I would add the props.onUpdate; so each time the user selects/unselect a role it would save. Is this the behavior we want?

[LamVu1]

@duckbytes Can you take a look at this?

[duckbytes]

It looks much nicer, thanks @LamVu1 !

I have some suggestions:

At the moment on the User profile, the popup form also changes state on the form underneath. It would be better I think to maintain state separately on the profile and the popup form (in new components). Then if the user clicks OK, the data is saved and state is updated on the profile after with the new data.

You could consider using DataStore.observe to update the state on the profile page (in UserDetail.js). This means when you use DataStore.save, the profile will update itself (and also respond to any remote changes).

In that case it's simpler to have the separate onConfirm functions just call DataStore.save with the new data, and there's no need to maintain the toChange ref or send data down a prop function.

This simplifies things nicely, that is made possible by using the popup forms instead!

I think it would be good to have descriptive headers opposite the edit icons. Like Contact Information, Address, etc.

You could replace the Name and Display Name section and put Name into the next section under contact information. Then the display name can be editable by itself (with the layout of display name opposite the edit icon staying as it is now).

Sorry if this is a lot. Let me know if you have any questions or want to talk through some of these points sometime.

[LamVu1]

@duckbytes Can you take a look at this?

[duckbytes]

hey @LamVu1

I like the vehicle and location pages now.

A few points of feedback:

• The mobile number on user page does not use valid telephone number verification.
• If you change TextfieldControlled to TextFieldUncontrolled, it will prevent the user from entering letters into the telephone fields too
• Best to use TextFieldUncontrolled only for telephone numbers and TextField for anything else. I was using those components before for any fields but I'm moving away from it as it seems simpler this way.
• The way of editing roles could be neater I think. I would get rid of the save and cancel buttons and just toggle editing mode with the edit icon. Save the new roles with an API call when the edit icon is clicked again and disable edit mode. You can also put secondary colour on the edit icon when edit mode is enabled (like it does in the task overview page).
• You could consider using the networkStatusSelector in src/redux/Selectors.js to disable changing the roles if the logged in user is offline (although the functionality there is only partial, but eventually Amplify should fix it in their library). Add a tooltip to say why the edit icon is disabled.
• Then you could put the save roles logic into a separate function. This prevents an error being thrown when saving other fields if the user is offline.
• Some console.logs to be cleaned up

Would you like to write some tests for these components as well?

[duckbytes]

Hey @LamVu1

Thanks for the work.

On the user detail page I can't edit their address for some reason. Clicking the edit icon doesn't make anything come up.

I still get an error when saving changes on the user detail while offline. My guess is that it's still trying to save the roles with a graphql call when it doesn't need to.

On the location edit view there isn't any way to edit the location name. It might be good to make it a separate edit icon in the same way that the user view has a separate edit icon for changing the display name.

Don't completely restrict editing the roles when the offline status is set to true. I would just put a warning that it may not work (the offline status indicator isn't reliable enough to enforce the UI).

Thanks again. Let me know if you have any questions.

[LamVu1]

[LamVu1]

@duckbytes I fixed most comments beside, Saving the roles, I'm not sure if any changes are being made when I change roles.