feat(kit): allow configuration of default options for input-count #675
Conversation
KarimovDev
requested review from
MarsiBarsi,
nsbarsukov,
vladimirpotekhin and
waterplea
as code owners
KarimovDev
force-pushed
the
feat-input-count-options
branch
from
db557bc
to
385b80d
Compare
|
|
||
| export interface InputCountOptions { | ||
| readonly icons: Readonly<{ | ||
| up: PolymorpheusContent<TuiContextWithImplicit<TuiSizeL>>; |
There was a problem hiding this comment.
Buttons are rather tiny, they always have size "s". How about we simplify it by keeping context {} — this way you can just pass it to buttons' icon input, as it is already polymorpheus.
There was a problem hiding this comment.
But buttons' height gets smaller when input-count uses "s" size. And this polymorpheus context is taken into account textfield size
There was a problem hiding this comment.
I meant that you probably cannot fit a big icon anyway, even for a big input size. Template complexity really increases now and I don't want this token option feature to become an overkill.
There was a problem hiding this comment.
Ok. I agree about template complexity. I've changed it to {} context
| down: PASSWORD_ICON_DOWN, | ||
| }, | ||
| appearance: 'textfield', | ||
| hideButtons: false, |
There was a problem hiding this comment.
Right now we force hide buttons in tables by comparing appearance to table. That's rather hardcode. @vladimirpotekhin @nsbarsukov what do you think, it probably makes sense to alter this token in Table directive now? With a factory, skipSelf and spread with hideButtons set to true? Sounds like that would be more idiomatic.
There was a problem hiding this comment.
In this case someone can override token by mistake if they use nested components inside the table. Or am I wrong?
There was a problem hiding this comment.
In this case, if somebody uses component inside table and for some reason want to change it, they can create a directive that would override it, yes. I don't think that's bad.
There was a problem hiding this comment.
Ok. I've already done this as an example. @vladimirpotekhin @nsbarsukov you can check it right here
There was a problem hiding this comment.
@waterplea there is also tuiTableMode directive with TUI_TEXTFIELD_APPEARANCE token. Should we add TUI_INPUT_COUNT_OPTIONS here as well? In this case, should we move TUI_INPUT_COUNT_OPTIONS into core package?
There was a problem hiding this comment.
No, that directive was there before we introduced table component. I believe we might end up just dropping it in the next major release.
There was a problem hiding this comment.
Oops, @KarimovDev I see this directive is still actually in use on the demo and that fails the screenshots. Let's copy-paste this factory for the time being then.
KarimovDev
force-pushed
the
feat-input-count-options
branch
3 times, most recently
from
a51859f
to
6997d54
Compare
KarimovDev
force-pushed
the
feat-input-count-options
branch
from
6997d54
to
729143c
Compare
Pull request was closed ✔️All saved screenshots (for current PR) were deleted 🗑️ |
|
|
Co-authored-by: Dima Karimov <d.karimov@aitarget.com>
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Partially implements #133
What is the new behavior?
Default input-count options can be set via a provider. This is a partial implementation of #133
Does this PR introduce a breaking change?
Other information