Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add remediation content #1114

Merged
merged 4 commits into from
Jan 17, 2023
Merged

Add remediation content #1114

merged 4 commits into from
Jan 17, 2023

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Jan 16, 2023

Description

Adds the remediation content in the dialog that opens when clicking on a check row in ExecutionResults

remediation-suggestion-dialog

How was this tested?

Automatic test added

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on this @nelsonkopliku


export const getCheckRemediation = (catalog, checkID) => {
const check = findCheck(catalog, checkID);
return check?.remediation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess having null or undefined here doesn't make much difference

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess so 😄 But if any preference I can change it to if whathever

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to an if for consistency with getCheckDescription

@@ -108,6 +111,13 @@ const prepareStateData = (checkExecutionStatus) => {

describe('ExecutionResults', () => {
it('should render ExecutionResults with successfully fetched results', async () => {
window.IntersectionObserver = jest.fn().mockImplementation(() => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this IntersectionObserver for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Headlessui dialog related need. Without it we get an error (which I don't remember now) and google helped me 😄

@arbulu89
Copy link
Contributor

By the way @nelsonkopliku , I noticed in the gif that we are not rendering the modal title as markdown. Yet another good to have fix hehe

@nelsonkopliku
Copy link
Member Author

@arbulu89 made also the dialog title a markdown. Thanks! I missed it 🚀

@nelsonkopliku nelsonkopliku force-pushed the add-remediation-content branch from 4734083 to b4708ce Compare January 17, 2023 08:39
Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Just a small idea about the Modal component.
For now we need to click somewhere outside the Modal, this may confuse the user on the first usage.
What do you think to add an Exit/Close button to it ?

@nelsonkopliku
Copy link
Member Author

What do you think to add an Exit/Close button to it ?

@EMaksy I think it is a good idea, however, since we are going to revisit this soon I'd keep the close button as part of that improvement. Makes sense?

@nelsonkopliku nelsonkopliku merged commit 409dcec into main Jan 17, 2023
@nelsonkopliku nelsonkopliku deleted the add-remediation-content branch January 17, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants