Skip to content

ui: tests for test section of overview pane#4083

Merged
maiamcc merged 4 commits into
masterfrom
maiamcc/test-section-tests
Jan 19, 2021
Merged

ui: tests for test section of overview pane#4083
maiamcc merged 4 commits into
masterfrom
maiamcc/test-section-tests

Conversation

@maiamcc

@maiamcc maiamcc commented Jan 15, 2021

Copy link
Copy Markdown
Contributor

Hello @nicks, @hyu,

Please review the following commits I made in branch maiamcc/test-section-tests:

5c6b5ed (2021-01-15 12:29:33 -0500)
ui: tests for test section of overview pane

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@maiamcc
maiamcc requested review from hyu and nicks January 15, 2021 17:29
@maiamcc
maiamcc marked this pull request as draft January 15, 2021 17:30
@maiamcc

maiamcc commented Jan 15, 2021

Copy link
Copy Markdown
Contributor Author

Oh wait let me make the error reporting a little better

@maiamcc
maiamcc marked this pull request as ready for review January 15, 2021 17:40
@maiamcc

maiamcc commented Jan 15, 2021

Copy link
Copy Markdown
Contributor Author

...never mind, there's no clean way to do this, this level of verbosity in test output is fine--a user can go back through the stack trace to find where things went wrong if they need to :-/

Comment thread web/src/OverviewPane.tsx Outdated
<ServicesLabel>Pinned Resources</ServicesLabel>
</ServicesDividerRoot>
)
function PinnedResources(items: OverviewItem[]) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you make these React components that return React.Fragments?

Comment thread web/src/OverviewPane.tsx Outdated
)
function PinnedResources(items: OverviewItem[]) {
return items?.length ? (
<div className={"resources-container pinned"}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's get rid of all the CSS class names in this PR. None of them are used for styling. Once this is a React.Fragment, you should be able to use the react component itself as a selector.

@maiamcc
maiamcc requested a review from nicks January 15, 2021 21:23
@maiamcc

maiamcc commented Jan 15, 2021

Copy link
Copy Markdown
Contributor Author

thanks for the tip -- this should be better!

Comment thread web/src/OverviewPane.tsx Outdated
export function PinnedResources(props: ResourceProps) {
return props.items?.length ? (
<React.Fragment>
<ServicesDividerRoot className={"ServicesDivider pinned"}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you should be able to get rid of these classes too now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

whoops good catch

@maiamcc
maiamcc merged commit 405d54a into master Jan 19, 2021
@maiamcc
maiamcc deleted the maiamcc/test-section-tests branch January 19, 2021 22:20
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.

2 participants