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(kit): default options configuration for select, multi-select, combo-box #1352

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

GinMitch
Copy link
Contributor

@GinMitch GinMitch commented Feb 9, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows Conventional Commits
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactoring
  • Code style update
  • Build or CI related changes
  • Documentation content changes

What is the current behavior?

Issue #133 is not fully implemented.

What is the new behavior?

Partial implementation of #133 for data-list-wrapper, select, multi-select, and combo-box components.

Does this PR introduce a breaking change?

  • Yes
  • No

@lumberjack-bot
Copy link

lumberjack-bot bot commented Feb 9, 2022

Pull request was closed ✔️

All saved screenshots (for current PR) were deleted 🗑️

Copy link
Collaborator

@waterplea waterplea left a comment

Choose a reason for hiding this comment

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

Thank you very much! Please also add options to ComboBox. I also wonder if we can have some master token for those things so they are overridable in one place - because they are all about the same thing.

@GinMitch GinMitch changed the title feat(kit): default options configuration for select, multi-select feat(kit): default options configuration for select, multi-select, combo-box Feb 9, 2022
@GinMitch GinMitch requested a review from waterplea February 9, 2022 13:42
@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2022

Codecov Report

Merging #1352 (ff1647f) into main (e9384bf) will increase coverage by 0.09%.
The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1352      +/-   ##
==========================================
+ Coverage   64.67%   64.77%   +0.09%     
==========================================
  Files         766      770       +4     
  Lines        8754     8778      +24     
  Branches     1742     1742              
==========================================
+ Hits         5662     5686      +24     
  Misses       2674     2674              
  Partials      418      418              
Flag Coverage Δ
addon-charts 71.98% <ø> (ø)
addon-doc 25.84% <ø> (ø)
addon-editor 46.05% <ø> (ø)
addon-mobile ∅ <ø> (∅)
addon-table 81.39% <ø> (ø)
addon-tablebars ∅ <ø> (∅)
cdk 82.33% <ø> (ø)
core 69.07% <ø> (ø)
kit 63.81% <95.12%> (+0.21%) ⬆️
summary 64.77% <95.12%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/data-list-wrapper/data-list-wrapper.component.ts 75.00% <80.00%> (+1.66%) ⬆️
projects/kit/components/select/select.component.ts 62.50% <83.33%> (+3.87%) ⬆️
...ects/kit/components/combo-box/combo-box-options.ts 100.00% <100.00%> (ø)
...ts/kit/components/combo-box/combo-box.component.ts 54.41% <100.00%> (+1.38%) ⬆️
...it/components/multi-select/multi-select-options.ts 100.00% <100.00%> (ø)
.../components/multi-select/multi-select.component.ts 61.72% <100.00%> (+0.96%) ⬆️
projects/kit/components/select/select-options.ts 100.00% <100.00%> (ø)
projects/kit/tokens/items-handlers.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9384bf...ff1647f. Read the comment docs.

Copy link
Collaborator

@waterplea waterplea left a comment

Choose a reason for hiding this comment

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

@vladimirpotekhin @splincode @mullinsmikey
stringify repeats itself in every token. Also identityMatcher and disabledItemHandler appear in multiple places. Maybe we can create TUI_ITEMS_HANDLERS token with those three and reuse it across relevant components? I think if we do that, we can drop options for DataListWrapper (it will only have size which tends to change from one use to another so changing the default is not that helpful) and Select (it will only have valueContent and with added stringify it might also be useless to change the default on valueContent). We can keep options for MultiSelect (expandable and valueContent) and ComboBox (strictMatcher, valueContent and strict). What do you think? @mullinsmikey sorry to keep adding more work and refactor to this task, just want to make sure we get the best and most flexible solution here 🙂

@GinMitch
Copy link
Contributor Author

Don't worry, I totally get it
Come up with the decision about this suggestion and I'll work on it. For me, this seems pretty good.

@splincode
Copy link
Member

@waterplea do you mean?

{
   provide: TUI_ITEMS_HANDLERS,
   useFactory: myCustomHandler,
   multi: true
},
{
   provide: TUI_ITEMS_HANDLERS,
   useFactory: identityMatcher,
   multi: true
},
{
   provide: TUI_ITEMS_HANDLERS,
   useFactory: stringify,
   multi: true
},

@waterplea
Copy link
Collaborator

@waterplea do you mean?

{
   provide: TUI_ITEMS_HANDLERS,
   useFactory: myCustomHandler,
   multi: true
},
{
   provide: TUI_ITEMS_HANDLERS,
   useFactory: identityMatcher,
   multi: true
},
{
   provide: TUI_ITEMS_HANDLERS,
   useFactory: stringify,
   multi: true
},

No, I just meant a separate token with this interface:

export interface TuiItemHandlers<T> {
    readonly stringify: TuiStringHandler<T>;
    readonly identityMatcher: TuiIdentityMatcher<T>;
    readonly disabledItemHandler: TuiBooleanHandler<T>;
}

@vladimirpotekhin
Copy link
Member

Separate token TUI_ITEMS_HANDLERS looks good to me, let's do it

@waterplea
Copy link
Collaborator

Ok, @mullinsmikey please go ahead and implement it when you got time.

@GinMitch
Copy link
Contributor Author

Strange how Cypress job is failing with npm ERR! Unexpected token < in JSON at position 38 even if I didn't change any .json file in this branch.
Could you help me investigate and resolve this issue?

@splincode
Copy link
Member

@mullinsmikey hello, I see you have problem with your branch

image

please, rebase your branch manually relative to main branch

@GinMitch GinMitch force-pushed the feat/select-options branch from b4c3c0f to ff1647f Compare February 11, 2022 10:27
@GinMitch
Copy link
Contributor Author

@splincode thanks for your help!

@splincode
Copy link
Member

@mullinsmikey sorry, about that, but some files incorrect

image

@GinMitch GinMitch force-pushed the feat/select-options branch from ff1647f to 20b4f2a Compare February 11, 2022 11:03
@GinMitch
Copy link
Contributor Author

@splincode yeah, I completely forgot that rebase is applied to each commit. Had to squash the whole branch

@splincode
Copy link
Member

@mullinsmikey thank you very much for your patience

@GinMitch
Copy link
Contributor Author

@splincode is it OK that there are 2 instances of cypress being launched simultaneously and one of them is failing?

@waterplea waterplea merged commit 04dc1ab into taiga-family:main Feb 11, 2022
@well-done-bot
Copy link

well-done-bot bot commented Feb 11, 2022

'Well done'

@GinMitch GinMitch deleted the feat/select-options branch February 11, 2022 14:22
vladimirpotekhin pushed a commit that referenced this pull request Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants