Skip to content

Conversation

@mikeprosserni
Copy link
Collaborator

@mikeprosserni mikeprosserni commented Jun 13, 2025

What does this Pull Request accomplish?

This just adds two more samples, for a total of three.

Why should this Pull Request be merged?

This will make it easier to test with multiple panels.

What testing has been done?

I tested by running all three panels at the same time:

image

image

image

@mikeprosserni mikeprosserni marked this pull request as ready for review June 13, 2025 15:38
@mikeprosserni mikeprosserni requested a review from csjall as a code owner June 13, 2025 15:38
@mikeprosserni mikeprosserni enabled auto-merge (squash) June 13, 2025 15:38
@github-actions
Copy link
Contributor

github-actions bot commented Jun 13, 2025

Test Results

   10 files  ±0     10 suites  ±0   18s ⏱️ -1s
  150 tests ±0    150 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 470 runs  ±0  1 470 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit c3dad08. ± Comparison against base commit d2a957b.

♻️ This comment has been updated with latest results.

@mikeprosserni mikeprosserni disabled auto-merge June 13, 2025 21:17
@mikeprosserni mikeprosserni enabled auto-merge (squash) June 13, 2025 21:21
csjall
csjall previously requested changes Jun 13, 2025
Copy link
Collaborator

@jfriedri-ni jfriedri-ni left a comment

Choose a reason for hiding this comment

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

I made some documentation suggestions to replace passive voice with active voice.

@mikeprosserni mikeprosserni disabled auto-merge June 23, 2025 15:35
@jfriedri-ni jfriedri-ni requested review from csjall and removed request for csjall June 23, 2025 17:33
@jfriedri-ni jfriedri-ni dismissed csjall’s stale review June 23, 2025 17:43

Johann is out of office, and Joe is taking his role for this PR.

Copy link
Collaborator

@jfriedri-ni jfriedri-ni left a comment

Choose a reason for hiding this comment

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

Consider updating (or deleting) the screenshots in the PR description or moving them as assets for the readmes so users can know whether "they got it working" correctly.

Food for thought later on...

  • In the logic script, we call create_panel() and store the retval in a variable named panel
  • In the visualization script, we call initialize_panel() and store the retval in a variable named panel
  • But the panels are not the same class

One impression a reader may have is "I should call create, and then call initialize," but that would put them on a confusing path. Perhaps we can use create_visualization() or similar rather than initialize_panel() to hint that these functions are not meant to be used together in the same file.

@mikeprosserni mikeprosserni merged commit dcaf336 into main Jun 23, 2025
14 checks passed
@mikeprosserni mikeprosserni deleted the users/mprosser/add-more-samples branch June 23, 2025 20:00
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.

5 participants