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

Adding show report skeleton to UI #1198

Merged
merged 6 commits into from
Sep 6, 2023
Merged

Adding show report skeleton to UI #1198

merged 6 commits into from
Sep 6, 2023

Conversation

bdomen-ggl
Copy link
Contributor

@bdomen-ggl bdomen-ggl commented Sep 1, 2023

Adding the core pieces for the show report view.
Will add the actual UI components in the next PR since there's quite a few.

@wfa-reviewable
Copy link

This change is Reviewable

@bdomen-ggl bdomen-ggl force-pushed the bdomen-react-getReport branch from 3d93af9 to 64d1938 Compare September 1, 2023 17:27
@bdomen-ggl bdomen-ggl marked this pull request as ready for review September 1, 2023 17:32
Copy link
Contributor Author

@bdomen-ggl bdomen-ggl left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)


experimental/reporting-ui/src/main/react/reporting-ui/view/show_report/show_report_view.tsx line 43 at r1 (raw file):

  if (TERMINAL_STATES.includes(report.status)) {
    return <div>{report.name}</div>

Will add the components to render the report and update the UI model in the next PR.

We don't currently have designs for the other cases (error, loading, and a non-completed report).

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bdomen-ggl and @stevenwarejones)


experimental/reporting-ui/src/main/react/reporting-ui/view/show_report/show_report_view.tsx line 22 at r1 (raw file):

import { TERMINAL_STATES } from '../../model/reporting';

export const ShowReportView = () => {

nit: Why the "Show"? Why not just ReportView?

Code quote:

ShowReportView

Copy link
Contributor Author

@bdomen-ggl bdomen-ggl left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SanjayVas and @stevenwarejones)


experimental/reporting-ui/src/main/react/reporting-ui/view/show_report/show_report_view.tsx line 22 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: Why the "Show"? Why not just ReportView?

Yeah I was thinking about that too. But was wondering if I should then call the files/classes like "ReportViewView" and "ReportViewViewModel" - "ReportView" for the base description, and View and View Model as specifiers. The double "View" seemed odd. So was thinking about renaming the base description so terms don't overlap.

Or you mean like JUST "ReportView" and "ReportViewModel" without an additional description to Report? I can do that.

@bdomen-ggl bdomen-ggl force-pushed the bdomen-react-getReport branch from 5278fd5 to 63beaa5 Compare September 5, 2023 14:36
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


experimental/reporting-ui/src/main/react/reporting-ui/view/show_report/show_report_view.tsx line 22 at r1 (raw file):

Or you mean like JUST "ReportView" and "ReportViewModel" without an additional description to Report? I can do that.

Yes. ReportView meaning the view for a single Report. We could rename to add other modifiers if end up with multiple views for a single Report, e.g. ReportSummaryView and ReportDetailsView.

Copy link
Contributor Author

@bdomen-ggl bdomen-ggl left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


experimental/reporting-ui/src/main/react/reporting-ui/view/show_report/show_report_view.tsx line 22 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Or you mean like JUST "ReportView" and "ReportViewModel" without an additional description to Report? I can do that.

Yes. ReportView meaning the view for a single Report. We could rename to add other modifiers if end up with multiple views for a single Report, e.g. ReportSummaryView and ReportDetailsView.

Done.

Sorry, forgot to publish.

@bdomen-ggl bdomen-ggl force-pushed the bdomen-react-getReport branch from 63beaa5 to 4948f2f Compare September 6, 2023 16:40
@bdomen-ggl bdomen-ggl merged commit 6df63c1 into main Sep 6, 2023
@bdomen-ggl bdomen-ggl deleted the bdomen-react-getReport branch September 6, 2023 20:44
ple13 pushed a commit that referenced this pull request Aug 16, 2024
Adding the core pieces for the show report view.
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.

3 participants