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 SimpleDocumentViewerModal to display analysis details #1072

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

sjd78
Copy link
Member

@sjd78 sjd78 commented Jun 27, 2023

Instead of opening a new browser window to show a user a json formatted analysis details document, open a modal on page with an embedded and enhanced CodeEditor.

The SimpleDocumentViewerModal component allows fetching and displaying the document as either a json or yaml document. Users can request the other format on screen. By using the @patternfly/react-code-editor component, a number of good things are enabled:

  • the content is displayed on page and is code colored per language
  • the contents can be copied to the clipboard
  • the contents can be save to a file
  • the user can easily view either the json or yaml format of the same document, as provided by the rest api endpoint

Resolves: #835
Resolves: https://issues.redhat.com/browse/MTA-773

@sjd78
Copy link
Member Author

sjd78 commented Jun 27, 2023

How it looked before

Firefox:
Screenshot from 2023-06-27 18-59-07

Chrome:
Screenshot from 2023-06-27 18-58-39

How it looks now

Firefox:
Screenshot from 2023-06-27 19-12-15

Chrome:
Screenshot from 2023-06-27 18-57-24

Swapping between yaml and json:

screencast-localhost_9000-2023.06.27-19_14_25.mp4

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 21.42% and project coverage change: -0.07 ⚠️

Comparison is base (7fd4726) 47.05% compared to head (b6694f4) 46.99%.

❗ Current head b6694f4 differs from pull request most recent head aeeba06. Consider uploading reports for the commit aeeba06 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1072      +/-   ##
==========================================
- Coverage   47.05%   46.99%   -0.07%     
==========================================
  Files         177      177              
  Lines        4416     4426      +10     
  Branches      983      986       +3     
==========================================
+ Hits         2078     2080       +2     
- Misses       2324     2332       +8     
  Partials       14       14              
Flag Coverage Δ
unitests 46.99% <21.42%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/src/app/api/rest.ts 53.98% <21.42%> (-1.04%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sjd78
Copy link
Member Author

sjd78 commented Jun 27, 2023

I did not add any unit tests (snapshot or otherwise) so far. I was trying to do it, but the testing framework does not like a component that does a useEffect() to grab data and update it's own display. I have not figured out how to successfully do that yet. As far as I got was for the component to render the empty state and snapshot it. Trying to get the component rendered and a snapshot with data it mock fetches has been elusive.

Net result is that Codecov does NOT like me right now.

Instead of opening a new browser window to show a user a json
formatted analysis details document, open a modal on page with
an embedded and enhanced CodeEditor.

The `SimpleDocumentViewerModal` component allows fetching and
displaying the document as either a json or yaml document.  Users
can request the other format on screen.  By using the
`@patternfly/react-code-editor` component, a number of good things
are enabled:
  - the content is displayed on page and is code colored per language
  - the contents can be copied to the clipboard
  - the contents can be save to a file
  - the user can easily view either the json or yaml format of the
    same document, as provided by the rest api endpoint

Signed-off-by: Scott J Dickerson <sdickers@redhat.com>
@@ -749,6 +757,13 @@ export const ApplicationsTableAnalyze: React.FC = () => {
}}
/>
)}

<SimpleDocumentViewerModal<Task | string>
Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

I was wondering why you chose to pass the fetch functions down as props rather than use them inside the component itself. Seems like since we are already using conditional logic to determine the language type here, then we can just use the appropriate fetch functions within that conditional? Does that make sense?

Either way, I like the use of typescript to make the component more extensible down the road. Maybe that was your intention. Just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially I wanted to have a more generic component that could be used elsewhere, and doing the fetch as a prop allows the location and methodology for the fetch can vary as needed.

In this PR, I eventually found 3 places that did a window.open or a Link with target="_blank". Two of those places used 1 URL, and the Link style was another. So more generic FTW! And playing with some generic typing with function type overloading got interesting.

One use:

<SimpleDocumentViewerModal<Task | string>
title={`Analysis details for ${application?.name}`}
fetch={getTaskById}
documentId={taskIdToView}
onClose={() => setTaskIdToView(undefined)}
/>

The other use:

<SimpleDocumentViewerModal<string>
title={`Analysis for ${application?.name}`}
fetch={getApplicationAnalysis}
documentId={appAnalysisToView}
onClose={() => setAppAnalysisToView(undefined)}
/>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

View report / task data in yaml
2 participants