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

Fixing some cosmetic issues with the output voltage measurement examp… #424

Merged

Conversation

DelpireNI
Copy link
Contributor

@DelpireNI DelpireNI commented Sep 28, 2023

…le measui.

What does this Pull Request accomplish?

Updated the measui for the Output Voltage Measurement example to fix cosmetic issues. We should now be more consistent with the other examples' UIs.

The Input and Output pin controls are now outside the grey boxes, and at the top like other examples.
The labels over the grey boxes have been renamed to Source configuration and Measurement configuration.

image

Why should this Pull Request be merged?

Consistency with other examples.
(#412)

What testing has been done?

Example still runs. This is just a cosmetic change.

…le measui.

Signed-off-by: Chris Delpire <chris.delpire@ni.com>
@DelpireNI
Copy link
Contributor Author

Other examples move Measurement type outside of the configuration box as well. I left it in because it allows the grey boxes to align and I believe this looks a bit better. The measurement type does feel like a "measurement configuration" to me.

@DelpireNI
Copy link
Contributor Author

It feels like there is too much margin at the top. I left it close to the existing UI's margin, but I'm happy to shift things up if others agree.

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

Test Results

     12 files  ±0       12 suites  ±0   2m 12s ⏱️ -23s
   228 tests ±0     199 ✔️ ±0    29 💤 ±0  0 ±0 
2 724 runs  ±0  2 360 ✔️ ±0  364 💤 ±0  0 ±0 

Results for commit dd7b12d. ± Comparison against base commit c008f98.

♻️ This comment has been updated with latest results.

@bkeryan
Copy link
Collaborator

bkeryan commented Sep 28, 2023

It feels like there is too much margin at the top. I left it close to the existing UI's margin, but I'm happy to shift things up if others agree.

I agree, the top/bottom margins are inconsistent and too big.

Also, the grey boxes have inconsistent left/bottom/right margins. (I'm ignoring top because of the label.)

Also the spacing between columns.

Signed-off-by: Chris Delpire <chris.delpire@ni.com>
Signed-off-by: Chris Delpire <chris.delpire@ni.com>
@DelpireNI
Copy link
Contributor Author

It feels like there is too much margin at the top. I left it close to the existing UI's margin, but I'm happy to shift things up if others agree.

I agree, the top/bottom margins are inconsistent and too big.

Also, the grey boxes have inconsistent left/bottom/right margins. (I'm ignoring top because of the label.)

Also the spacing between columns.

Okay I have made the margins more consistent and reduced the height.

Signed-off-by: Chris Delpire <chris.delpire@ni.com>
@DelpireNI DelpireNI requested a review from bkeryan September 28, 2023 18:12
@DelpireNI DelpireNI merged commit 1d1d060 into main Sep 28, 2023
@DelpireNI DelpireNI deleted the users/cdelpire/fix-output-voltage-measurement-cosmetic-issues branch September 28, 2023 21:07
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