-
Notifications
You must be signed in to change notification settings - Fork 13
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 header, cards, and table to Report view #1203
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 22 files at r1, 7 of 10 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bdomen-ggl)
experimental/reporting-ui/src/main/react/reporting-ui/util/BUILD.bazel
line 10 at r3 (raw file):
js_library( name = "util",
If you're using the default target name for the package, use a glob. Otherwise, name the target more specifically. In general, prefer more granular targets if not everything from the package will be used together.
Suggestion:
formatting
experimental/reporting-ui/src/main/react/reporting-ui/util/BUILD.bazel
line 12 at r3 (raw file):
name = "util", srcs = [ "numberWithMagnitude.ts",
Consistent file naming pattern (lower_snake_case). Also naming suggestion for a file with formatting functions.
Suggestion:
formatting.ts
experimental/reporting-ui/src/main/react/reporting-ui/util/numberWithMagnitude.ts
line 23 at r3 (raw file):
}); export const numberWithMagnitude = (num: number, decimals: number, trailingZeros: boolean = false) => {
nit: naming that makes what this is doing more obvious
Suggestion:
formatNumberWithMagnitude
1f1c538
to
bbf56bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 19 of 24 files reviewed, 3 unresolved discussions (waiting on @SanjayVas)
experimental/reporting-ui/src/main/react/reporting-ui/util/BUILD.bazel
line 10 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
If you're using the default target name for the package, use a glob. Otherwise, name the target more specifically. In general, prefer more granular targets if not everything from the package will be used together.
Gotcha sounds good.
Renamed to formatting
experimental/reporting-ui/src/main/react/reporting-ui/util/BUILD.bazel
line 12 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Consistent file naming pattern (lower_snake_case). Also naming suggestion for a file with formatting functions.
Oops good one. Had a brain fart on that one. Also like the name suggestion.
Changed names to formatting.ts
.
experimental/reporting-ui/src/main/react/reporting-ui/util/numberWithMagnitude.ts
line 23 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
nit: naming that makes what this is doing more obvious
Yeah good name suggestion.
Changed the name.
b61db23
to
ca4003b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)
ca4003b
to
2518e5b
Compare
Adding components to the Report view - minus charts
Adding components to the Report view:
Graphs will come as a follow up PR.