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

fix(protocol-designer, component): fixes RadioButton and update path tooltip #16635

Merged
merged 17 commits into from
Nov 1, 2024

Conversation

syao1226
Copy link
Collaborator

@syao1226 syao1226 commented Oct 30, 2024

fix RQA-3270, re AUTH-862

Overview

Updating the RadioButton component to fix the "double-clicking" behavior when quickly clicking through different paths in the Transfer step. Also updating the GIF images for path animation.

Test Plan and Hands on Testing

Changelog

  • moved styled components outside the RadioButton component to prevent them from being created dynamically
  • updated gifs

Review requests

Risk assessment

@syao1226 syao1226 changed the title fix(protocol-designer): fixes RadioButton and update path tooltip fix(protocol-designer, component): fixes RadioButton and update path tooltip Oct 30, 2024
@syao1226 syao1226 force-pushed the protocol_designer-path-double-click branch from 1f259f7 to 0f513ca Compare October 30, 2024 21:10
@syao1226 syao1226 marked this pull request as ready for review October 31, 2024 14:48
@syao1226 syao1226 requested review from a team as code owners October 31, 2024 14:48
@syao1226 syao1226 requested review from jerader and koji and removed request for a team October 31, 2024 14:48
Comment on lines 114 to 120
css={
isSelected
? SELECTED_BUTTON_STYLE
: disabled
? DISABLED_BUTTON_STYLE
: AVAILABLE_BUTTON_STYLE
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we avoid using a nested ternary operator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh i'll create a helper function to handle this

disabled: boolean
largeDesktopBorderRadius: boolean
isLarge: boolean
maxLines?: number | null
Copy link
Contributor

Choose a reason for hiding this comment

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

need to support null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'm not sure. does setting it to null means it has no line limit? but it's an optional prop

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can do something like below since maxLines is used in ODD only and there should be 1 line text even we don't use maxLines.

@jerader @ncdiehl

maxLines?: number


{...,
  maxLines = 1,
  ...
}

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

lgtm
I left one comment.

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.

lgtm! nice work!

@syao1226 syao1226 merged commit 7e4a6bb into edge Nov 1, 2024
34 checks passed
@syao1226 syao1226 deleted the protocol_designer-path-double-click branch November 1, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants