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

feat: better chain selection concept #4596

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

xeno097
Copy link
Contributor

@xeno097 xeno097 commented Sep 30, 2024

Description

This PR implements an updated view of the multi-chain selection step that now allows searching for chains in the current list

Before:

image

image

After:

image

image

image

Drive-by changes

  • Updated the runMultiChainSelectionStep function to take as param an object instead of a list of params because the list was growing larger

Related issues

Backward compatibility

  • Yes

Testing

  • Manual
    • Manual testing has also been conducted on Windows to see if Bug: chain selection for warp init doesn't work on windows  #4508 was solved. In this case, the following were discovered:
      • The chain selection is unusable on gitbash.
      • The chain selection works perfectly fine using powershell and cmd. I assume the issue is linked to how gitbash handles inputs or simulates a UNIX environment on Windows. CLI users on windows should use either one of these options

Copy link

changeset-bot bot commented Sep 30, 2024

🦋 Changeset detected

Latest commit: 15c7df3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@hyperlane-xyz/cli Minor
@hyperlane-xyz/core Minor
@hyperlane-xyz/ccip-server Minor
@hyperlane-xyz/github-proxy Minor
@hyperlane-xyz/helloworld Minor
@hyperlane-xyz/infra Minor
@hyperlane-xyz/sdk Minor
@hyperlane-xyz/utils Minor
@hyperlane-xyz/widgets Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.89%. Comparing base (330b058) to head (15c7df3).
Report is 39 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4596   +/-   ##
=======================================
  Coverage   73.89%   73.89%           
=======================================
  Files         100      100           
  Lines        1421     1421           
  Branches      180      180           
=======================================
  Hits         1050     1050           
  Misses        350      350           
  Partials       21       21           
Components Coverage Δ
core 84.61% <ø> (ø)
hooks 75.71% <ø> (ø)
isms 79.20% <ø> (ø)
token 88.23% <ø> (ø)
middlewares 77.39% <ø> (ø)

Copy link
Contributor

@ltyu ltyu left a comment

Choose a reason for hiding this comment

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

Great progress. I love that you thought about other potential use cases. my feedback is generally, I would prefer to keep thing simpler, until we need them

for example, i noticed that there are multiple help modes, but 1) it's unclear how to use them, and 2) if we'll ever need them because we only use always in runMultiChainSelectionStep

typescript/cli/src/utils/chains.ts Outdated Show resolved Hide resolved
typescript/cli/src/utils/chains.ts Outdated Show resolved Hide resolved
typescript/cli/src/utils/input.ts Outdated Show resolved Hide resolved
typescript/cli/src/utils/input.ts Outdated Show resolved Hide resolved
typescript/cli/src/utils/input.ts Outdated Show resolved Hide resolved
typescript/cli/src/utils/input.ts Outdated Show resolved Hide resolved
typescript/cli/src/utils/input.ts Outdated Show resolved Hide resolved
return key.name === 'down';
}

export const searchableCheckBox = createPrompt(
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't really tell that i'm able to search, until i start typing. Is there a good way to let the user know that it's searchable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a hint to the help message should be okay I think. We already do the same for telling the user to use tabs to select and enter to complete the selection. Examples:

Before
image

After
image

@xeno097 xeno097 requested a review from ltyu October 9, 2024 22:51
typescript/cli/src/utils/input.ts Show resolved Hide resolved
checkboxTheme,
config.theme,
);
const firstRender = useRef(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

So strange to see these react hooks here. I wondered if inquirer was using react under the hood but actually they implemented these themselves! https://github.com/SBoudrias/Inquirer.js/blob/main/packages/core/src/lib/use-memo.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was surprised at first too! Then I looked at the package code and saw that they implemented their own. super cool!

typescript/cli/src/utils/input.ts Outdated Show resolved Hide resolved
@jmrossy
Copy link
Contributor

jmrossy commented Oct 10, 2024

Mostly looks good to me so far!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

feat(cli): Better chain selection
3 participants