UI: Add macOS settings (profiles) indicator and modal with data table#9809
Conversation
There was a problem hiding this comment.
Do you need to use a callback here? Maybe try this instead:
setShowMacSettingsModal((prevState) => !prevState)
There was a problem hiding this comment.
this was just a pattern I saw elsewhere and followed in lieu of any other input. I'll do that.
There was a problem hiding this comment.
@gillespi314 I'm actually now remembering, @ghernandez345 doesn't like directly passing set{State} s to other components and prefers a handler of some sort, so this is in line with that. I am curious as to why he prefers that though?
There was a problem hiding this comment.
Yeah, I've seen both approaches and defer to Gabe if his preference is to always define handlers.
In that case you can still avoid the useCallback, I think.
const toggleMacSettingsModal = () => setShowMacSettingsModal((prevState) => !prevState);
There was a problem hiding this comment.
useState guarantees that the setter is referentially stable.
React guarantees that setState function identity is stable and won’t change on re-renders. This is why it’s safe to omit from the useEffect or useCallback dependency list.
But the toggleMacSettingsModal handler function will get recreated each render. And I see that it get passed as a prop to child components. This type of set up can result in performance problems when the child component needs intensive calculations to render. https://beta.reactjs.org/reference/react/useCallback#skipping-re-rendering-of-components. So you can add useCallback here to cache this particular handler prop although it might be overkill. Also note that other prop changes can still trigger the child to re-render so this alone isn't a silver bullet for performance problems if the other props aren't being tightly controlled as well. If you do go with useCallback on the handler here, you can use [] for the dependencies if you use the prevState updater function that I suggested.
There was a problem hiding this comment.
See related comment to DeviceUserPage
There was a problem hiding this comment.
Since these are basically static (i.e. there are no parameters to this function), you can eliminate the function and just export the tableHeaders constant.
There was a problem hiding this comment.
Based on the API mock, I don't think you will need this since it doesn't look like you need to do any transformation of the data.
There was a problem hiding this comment.
You might want to use the all caps format here: SETTINGS_STATUS_OPTIONS
There was a problem hiding this comment.
This might be a good candidate for its own React component.
There was a problem hiding this comment.
For sure. I have a MacSettingsIndicator component for the DataTable cells, which is pretty much the same format as this and I'll use here too.
Could also be combined with the "StatusIndicator" component that's used elsewhere, but that's blowing this up a bit much, perhaps.
There was a problem hiding this comment.
Looks like this could still be undefined based on the IHostSummaryProps? I wonder if we should try to tighten up that interface definition.
Fun fact, optional chaining works with functions: toggleOSPolicyModal?.()
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not sure if we want to use type="submit" when there is no form being submitted? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-type
b46d3e8 to
161eb55
Compare
e8e109d to
74f1127
Compare
|
@gillespi314 Thanks for your review! I'll defer to you on this review since you had some feedback implemented. |
36d02f1 to
5ca43c3
Compare
Addresses #9413
Implements
https://www.loom.com/share/d1b66a3076b94bf2add4fcf8666649a4
Notes
To aid in reviewing, you'll probably want to focus on:
Currently using mock data. Once Update
GET /hosts/{id}andGET /device/{token}to include status of all MDM profiles applied to host #9599 is completed, Remove mock macOS profiles data from Host Details and Device User pages #9888 will change these components to use the real data2/21 - removed mock data. Until the API returns the host.mdm.profiles data, settings indicator and modal will not render
Checklist