Connect embed section of settings page to API #1329
Conversation
|
Looking at this now. |
| const handleError = e => { | ||
| setError(getErrorMessage(e)); | ||
| setLoading(false); | ||
| setTimeout(() => setError(null), 5000); |
There was a problem hiding this comment.
Since we are auto-dismissing this temporary error message, have we considered using a Material UI "toast" component that is designed for that purpose?
I am seeing some unexpected blinking and scroll behavior in Firefox when running the first test step.
There was a problem hiding this comment.
We're currently using react-toastify elsewhere in the app, so I've switched to that here in 9d913b8 - it definitely is an improvement in terms of workflow and appearance.
| /* eslint-disable react-hooks/exhaustive-deps */ | ||
| // Disabled exhaustive dependencies warning because we don't want to rerun | ||
| // in response to the loading state changing | ||
| // When the embed cofig or fields are updated, wait for 500ms. If no |
There was a problem hiding this comment.
Thanks for the catch - updated in 9d913b8
| } else { | ||
| createConfig(); | ||
| } | ||
| }, 500); |
There was a problem hiding this comment.
Did we choose 500ms for any specific reason?
I am seeing "polling" like behavior where a PUT requests are being made repeatedly. I was expecting that PUT requests would only be made in response to a field changing.
There was a problem hiding this comment.
The polling behavior was caused because the update and create functions are being recreated on each render. Removing them from the dependency array restored expected behavior. The 500ms delay is meant to prevent repeated calls to the database while the user is actively typing, but the actual duration of the debounce is fairly random.
| @@ -517,3 +517,5 @@ export const PPE_FIELD_NAMES = Object.freeze([ | |||
| 'ppe_contact_email', | |||
| 'ppe_website', | |||
| ]); | |||
|
|
|||
| export const OARFont = 'ff-tisa-sans-web-pro,sans-serif'; | |||
There was a problem hiding this comment.
There are a few other components that reference this font string. Consider spreading the use of this constant to those component to ease the burden on a future maintainer that needs to change the font.
There was a problem hiding this comment.
Updated in 5726f0f.
src/app/src/util/embeddedMap.js
Outdated
| color: embedConfig.color.length ? embedConfig.color : '#3d2f8c', | ||
| font: embedConfig.font.length ? embedConfig.font : OARFont, |
There was a problem hiding this comment.
Why are we using .length on these fields rather than a basic check for a falsey value? Could we ever end up in an exceptional situation because color or length are null/undefined?
There was a problem hiding this comment.
Updated in 9d913b8 and also moved some values to constants for clarity/DRYness. I was thinking that 0s would come out as 'false' but since the numbers are strings, that isn't actually an issue.
| column_name: f.columnName, | ||
| })); | ||
|
|
||
| const formatWidthForServer = ({ width, fullWidth }) => { |
There was a problem hiding this comment.
When I check the 100% box I am seeing the width field being set to 100.
There was a problem hiding this comment.
|
Ready for another look. |
jwalgran
added
the
task 004
Connects the Embed Configuration settings tab to the API, allowing data fetching, creation, and update. Additionally, adds some UI error handling, and updates the components to reflect API data structure.
TaiWilkin
force-pushed
the
tw/connect-embed-config
branch
from
5726f0f
to
680632a
Compare
Overview
Connects the Embed Configuration settings tab to the API, allowing
data fetching, creation, and update. Additionally, adds some UI error
handling, and updates the components to reflect API data structure.
A small, unrelated second commit fixes an error in the proptypes for Feature Flags.
Connects #1312
Demo
Testing Instructions
Checklist
fixup!commits have been squashed