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

refactor(app): create utils to format value for rtp #14720

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

koji
Copy link
Contributor

@koji koji commented Mar 22, 2024

Overview

create formatRunTimeParameterValue to format rtp value and replace a local function with that.

I noticed a small bug I made. On the UI, the following code looks fine at first glance.
However, if the data doesn't have suffix, the return has an unnecessary space.
ex
value: 6 without suffix -> '6 ' (including unnecessary space) but for this case we should return '6'

Screenshot 2024-03-22 at 8 00 00 PM
  case 'int':
  case 'float':
    return `${defaultValue.toString()} ${suffix}`

close AUTH-236

Test Plan

Desktop app

  • protocol details parameters tab
  • protocol run setup parameters tab

ODD

  • protocol details parameters tab
  • protocol setup parameters screen
  • view-only parameters screen

Changelog

  • create a utils file and add formatRunTimeParameterValue + its test

Review requests

If there is a better place this util function, please let me know.

Risk assessment

low

create formatRunTimeParameterValue to format rtp value and replacelocal function with that

close AUTH-236
@koji koji requested review from jerader and ncdiehl11 March 22, 2024 23:58
@koji koji marked this pull request as ready for review March 23, 2024 00:03
@koji koji requested a review from a team as a code owner March 23, 2024 00:03
@koji koji removed the request for review from a team March 23, 2024 00:06
? `${defaultValue.toString()} ${suffix}`
: defaultValue.toString()
case 'boolean':
return Boolean(defaultValue) ? 'On' : 'Off'
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe the util should take in t as a prop so On and Off can be a in i18n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the util function.

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

thanks for creating this util! Left a suggestion about adding t as a prop

@koji koji merged commit de14354 into edge Mar 25, 2024
20 checks passed
@koji koji deleted the refactor_rtp-range-formatter branch March 25, 2024 17:31
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
* refactor(app): create utils to format value for rtp
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