Updated policy details page - #47437
Conversation
GitHub Actions
Failure
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 38670-policy-status-page #47437 +/- ##
============================================================
- Coverage 67.23% 67.19% -0.04%
============================================================
Files 2954 2957 +3
Lines 227530 227715 +185
Branches 11683 11768 +85
============================================================
+ Hits 152982 153021 +39
- Misses 60712 60858 +146
Partials 13836 13836
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
579a265 to
536b30a
Compare
Closes #46909 - Show labels and automation fields as 'single' values. - Change layout of policy fields. - Show all labels and all automations using modals.
85d4bc6 to
9c4ee00
Compare
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
LGTM! I have some suggestions to follow our FE patterns, let me know what you think
| @@ -0,0 +1,2 @@ | |||
| .policy-automations-modal { | |||
There was a problem hiding this comment.
nit: I'd remove if no styles are needed, unless there's a follow-up where we fill this
|
|
||
| await user.click(screen.getByRole("button", { name: "Done" })); | ||
|
|
||
| expect(onClose).toHaveBeenCalled(); |
There was a problem hiding this comment.
nit:
| expect(onClose).toHaveBeenCalled(); | |
| expect(onClose).toHaveBeenCalledTimes(1); |
(so that we also make sure it's called only once)
| const renderLabelList = ( | ||
| labels: ILabelPolicy[], | ||
| scopeLabel: string, | ||
| description: string, | ||
| onLabelClick?: (labelId: number) => void | ||
| ) => ( | ||
| <div className={`${baseClass}__section`}> | ||
| <span className={`${baseClass}__scope-description`}> | ||
| {description} <b>{scopeLabel}</b> of these labels: | ||
| </span> | ||
| <ul className={`${baseClass}__label-list`}> | ||
| {labels.map((label) => ( | ||
| <li key={label.id} className={`${baseClass}__label-item`}> | ||
| {onLabelClick ? ( | ||
| <Button variant="link" onClick={() => onLabelClick(label.id)}> | ||
| {label.name} | ||
| </Button> | ||
| ) : ( | ||
| <span>{label.name}</span> | ||
| )} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Not a blocker, but I'd say let's have this declared as a react component instead of a plain JS function and have it at the bottom of the file (below the main component).
For reference, we agreed on introducing this pattern on this PR #46213 on the FE sync. (It hasn't been merged yet, so if you change this, you'll need to have an eslint exception rule at the top of the file.)
| ))} | ||
| </ul> | ||
| </div> | ||
| <Button variant="link" onClick={() => openLabelModal()}> |
There was a problem hiding this comment.
nit:
| <Button variant="link" onClick={() => openLabelModal()}> | |
| <Button variant="link" onClick={openLabelModal}> |
| .policy-label-modal { | ||
| &__label-list { | ||
| list-style: none; | ||
| padding: 0; | ||
|
|
||
| margin-top: $pad-small; | ||
| margin-bottom: $pad-large; |
There was a problem hiding this comment.
I think we should follow this pattern and remove margin-top and margin-bottom:
Therefore I'd use flex columns + gaps on both .policy-label-modal (to have spacing between the two sections) + on each of the sections if need to have some space between each of the descriptions and the label list.
| ): { | ||
| includeLabels?: ILabelPolicy[]; | ||
| includeScopeLabel?: string; | ||
| excludeLabels?: ILabelPolicy[]; | ||
| excludeScopeLabel?: string; |
There was a problem hiding this comment.
nit: we could Pick these props from the modal props.
| )} | ||
| {showLabelModal && storedPolicy && ( | ||
| <PolicyLabelModal | ||
| {...getLabelModalData(storedPolicy)} |
There was a problem hiding this comment.
nit: I'd explicitly pass the props (though it would require to perform the getLabelModalData(storedPolicy) call above the return statement). Anyways I'd still do that single call and you could reuse the same result for renderLabels as well (instead of calling it within that closure).
Ideally, renderLabels would be replaced by a child PolicyDetailLabels or just Labels component at the bottom of the file. Then PolicyDetailsPage would just call getLabelModalData(storedPolicy) once and pass the result to whichever components need it (PolicyLabelModal and PolicyDetailLabels / Labels).
**Related issue:** Closes #46909 - Show labels and automation fields as 'single' values. - Change layout of policy fields. - Show all labels and all automations using modals.
**Related issue:** Closes #46909 - Show labels and automation fields as 'single' values. - Change layout of policy fields. - Show all labels and all automations using modals.
2 participants
Related issue: Closes #46909
Screen.Recording.2026-06-11.at.1.07.54.PM.mov
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing