-
Notifications
You must be signed in to change notification settings - Fork 155
feat: add theme option #415
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
base: main
Are you sure you want to change the base?
Conversation
|
|
@example/basic • @example/changesets commit: |
|
the approach isn't far off, but i don't think it makes sense to have a single set of options each prompt has its own style. they only cross over on some things, such as the left guide/border. i'd expect interface CommonOptions<TStyle> {
// ...
style: TStyle;
}
// or
interface Whatever extends CommonOptions, StyleOptions<WhateverStyle> {
// ..
} |
| } | ||
|
|
||
| interface AutocompleteSharedOptions<Value> extends CommonOptions { | ||
| type AutocompleteSharedOptions<Value, TStyle> = CommonOptions<TStyle> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| type AutocompleteSharedOptions<Value, TStyle> = CommonOptions<TStyle> & { | |
| interface AutocompleteSharedOptions<Value> extends CommonOptions<AutocompleteStyle> { |
nobody should be changing the style type of any prompt
each prompt should expose its own style interface, or the base style interface
|
you're not far off but its not quite what i meant here: interface TextPromptStyle {
someTextPromptSpecificStyle: string;
anotherOne: string;
}
interface TextPromptOptions extends CommonOptions<TextPromptStyle> {
// ..
}every prompt would have its own style interface. if it doesn't have any of its own, it would use a the style will always be a flat object. the only reason you had to introduce deep merge logic is because you chose to make the shape nested. just don't bother - keep it flat and you can throw the deep merge stuff away.
|
|
Like this? // Before
interface CheckboxTheme {
selected?: {
active?: string;
inactive?: string;
};
unselected?: {
active?: string;
inactive?: string;
};
disabled?: string;
}
// After
interface CheckboxThemeFlat {
selectedActive?: string;
selectedInactive?: string;
unselectedActive?: string;
unselectedInactive?: string;
disabled?: string;
} |
|
yes exactly, but there'd be no such thing as a "checkbox theme" there would be a text prompt theme, a select prompt theme, etc. if any of those share some properties, just have an internal interface they both extend |
dreyfus92
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @hyperz111, amazing work on this! however, I would suggest a different approach given the scope of changes needed.
what @43081j is suggesting is a significant architectural refactor, the nested structure needs to be completely flattened. trying to tackle all 8 prompts at once makes this PR very difficult to review.
break this into separate PRs, starting with just the autocomplete prompt. once we get that pattern right and merged, the others will follow the same approach and be much easier to review.
for this PR, I'd suggest:
- focus only on
autocomplete.tsandautocompleteMultiselect.ts - implement the flat theme structure James outlined (no nesting, prompt-specific styles)
- remove
@fastify/deepmergeand use simple shallow merging like object spreading - get this pattern approved and merged first
then follow up with separate PRs for:
select.tsandmultiselect.tsconfirm.tspassword.tsandtext.tsgroup-multiselect.tsselect-key.ts
|
having another think about this. i wonder if we should have something like this: interface GlobalTheme {
formatGuide?: (str: string) => string;
}
interface ThemeOptions<T> {
theme?: T & GlobalTheme;
}
interface CommonOptions {
// existing common options interface
}
// ...
export interface TextTheme {
formatCancel?: (str: string) => string;
}
export interface TextOptions extends CommonOptions, ThemeOptions<TextTheme> {
message: string;
placeholder?: string;
defaultValue?: string;
initialValue?: string;
validate?: (value: string | undefined) => string | Error | undefined;
}so you'd use it like this: text({
theme: {
formatGuide: (str) => colors.red(str), // make the left guide line/border red
}
});the names need some bikeshedding but this pattern is what i had in mind. and fully agree we need to really strip this down to granular changes |
I don't want to do that, but if it's for making the review easy. Maybe i can |
In this PR, i add
themeoption to some prompts:autocompleteautocomplete-multiselectconfirmgroup-multi-selectmultiselectpasswordselect-key(NOT perfect)selecttextSeeThemeOptionsincommon.ts.fixes #345