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: renderChoices option #1350

Closed
wants to merge 3 commits into from
Closed

Conversation

matthyk
Copy link
Contributor

@matthyk matthyk commented Jan 18, 2024

Closes #1349.

Adds the option renderChoices to the checkbox prompt. Currently, only the output of the selected choices can be modified by this option. Maybe we should consider to let this function also modify the prefix and message value?

      return `${prefix} ${message} ${chalk.cyan(renderChoices(selectedChoices, items))}`;

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a3ab276) 94.48% compared to head (7ad4db7) 94.49%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1350   +/-   ##
=======================================
  Coverage   94.48%   94.49%           
=======================================
  Files          51       51           
  Lines        4423     4429    +6     
  Branches      771      772    +1     
=======================================
+ Hits         4179     4185    +6     
  Misses        239      239           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matthyk
Copy link
Contributor Author

matthyk commented Jan 18, 2024

@SBoudrias I have exported the Config type to use it in the tests. If we use, we can avoid the usage of any

@matthyk matthyk marked this pull request as ready for review January 18, 2024 00:38
@SBoudrias
Copy link
Owner

@matthyk I was wondering if you saw that PR to add full theming capacity over at #1323

Thinking about the changes introduced here, I'm reflecting the better approach might be with a theme. The theming PR is functional and close to ready; I was just hoping to wrap up #1334 before merging it since it'd conflict pretty bad with that PR.

I'd love to hear your thoughts on this!

@matthyk
Copy link
Contributor Author

matthyk commented Jan 21, 2024

@SBoudrias Themes are a great feature! This would make it easy to add a renderChoices functionality. If the types around DefaultTheme would be changed a bit, you could also implement the feature in a way that the renderChoices function has access to the current theme and you could even reuse the renderChoices function for multiple Themes.

type CheckboxTheme = {
  checkedIcon: string;
  uncheckedIcon: string;
  cursorIcon: string;
  disabled: (text: string) => string;
};

type Config<Value> = PromptConfig<{
  pageSize?: number;
  instructions?: string | boolean;
  choices: ReadonlyArray<Choice<Value> | Separator>;
  loop?: boolean;
  required?: boolean;
  theme?: Partial<
    Theme<CheckboxTheme> & {
      style: Partial<Theme<CheckboxTheme>['style']>;
    }
  >;
  renderChoices?: (this: Config<Value>['theme'], selected: Choice<Value>[], items: Item<Value>[]) => string
}>;
const config: Config<number> = {
  choices: [],
  message: "undefined",
  renderChoices: function renderChoices(selected: Choice<number>[], items: Item<number>[]): string {
    // `this` is typed as Partial<Theme<CheckboxTheme>
    return this?.disabled?("text") // The current typing of `theme` causes us to have to do this ugly double check
  }
}

@SBoudrias
Copy link
Owner

@matthyk I like this idea very much! Would you want to open PRs against the theme branch? (I'm guessing ~2 PRs for the type change, and one for adding renderChoice)

I'll work on wrapping up the password prompt change in ~a week.

@SBoudrias
Copy link
Owner

Slightly thinking back on theme + renderChoices. API wise I'd prefer we pass the theme as argument rather than implicitly with this.

@matthyk
Copy link
Contributor Author

matthyk commented Jan 28, 2024

It possibly offers better usability for the developers.
I'm still happy to implement the changes in two separate PRs, but unfortunately, I don't have time for that for another 2 weeks or so

@matthyk
Copy link
Contributor Author

matthyk commented Feb 21, 2024

superseded by #1360

@matthyk matthyk closed this Feb 21, 2024
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.

feat: transformChoice options for checkbox
2 participants