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.

Sign up for GitHub

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

Jump to bottom

fix: (Core|Platform) refactoring select component with cdk key manager, selection model #4511

Merged
merged 2 commits into from
Feb 3, 2021
Merged

fix: (Core|Platform) refactoring select component with cdk key manager, selection model #4511

merged 2 commits into from
Feb 3, 2021

Conversation

sKudum
Copy link
Contributor

@sKudum sKudum commented Jan 28, 2021

Please provide a link to the associated issue.

closes #4425

Please provide a brief summary of this pull request.

Refactored select component in core with cdks keymanager and selection model.

parallel this PR fixes build issues in platform due to this changes.

BREAKING CHANGE:

Not required
extendedBodyTemplate- can be deprecated- as it is handled internally by select.component, controlTemplate!=null or undefined is considered as extendedBodyTemplate as true.

Before:

<fd-select [(value)]="selectedValue"  [extendedBodyTemplate]="true" [controlTemplate]="customSelectTemplate">

Now:

<fd-select [(value)]="selectedValue" [controlTemplate]="customSelectTemplate">

Please check whether the PR fulfills the following requirements

Documentation checklist:

@netlify
Copy link

netlify bot commented Jan 28, 2021 •
edited
Loading

Deploy preview for fundamental-ngx ready!

Built with commit 92477d8

https://deploy-preview-4511--fundamental-ngx.netlify.app

JKMarkowski
JKMarkowski suggested changes Jan 29, 2021
View reviewed changes
Copy link
Contributor

@JKMarkowski JKMarkowski left a comment

Choose a reason for hiding this comment

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

Hello @sKudum, amazing job 🚀! This is exactly what we needed :) I added some minor comments regarding code

libs/core/src/lib/select/select-key-manager.service.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/option/option.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select-key-manager.service.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select-key-manager.service.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
@manjunathanagaraj manjunathanagaraj mentioned this pull request Jan 29, 2021
Closed
@sKudum sKudum force-pushed the core/selectkeyManager branch from 1c0a56a to f522d13 Compare January 29, 2021 14:32
@sKudum sKudum requested a review from JKMarkowski January 29, 2021 14:34
@manjunathanagaraj manjunathanagaraj added this to the Sprint 56 - ariba milestone Feb 1, 2021
dimamarksman
dimamarksman suggested changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@dimamarksman dimamarksman left a comment

Choose a reason for hiding this comment

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

God job,
Got several questions

libs/core/src/lib/select/option/option.component.ts Show resolved Hide resolved
libs/core/src/lib/select/option/option.component.ts Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select-selection-model.service.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.interface.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/select/select.component.ts Outdated Show resolved Hide resolved
@sKudum sKudum force-pushed the core/selectkeyManager branch from f522d13 to e7c7516 Compare February 1, 2021 17:17
@sKudum sKudum requested review from dimamarksman and fkolar February 1, 2021 17:18
@sKudum sKudum force-pushed the core/selectkeyManager branch from e7c7516 to 5c21ca5 Compare February 3, 2021 09:11
@sKudum sKudum requested a review from JKMarkowski February 3, 2021 09:12
sKudum added 2 commits February 3, 2021 16:02
@sKudum
adding cdk selectionmodel and keymanager
327ed51
@sKudum
addressing JKMarkowski comments
92477d8 
addressing frank and dimitry comments

addressing jedrzej second phase review comments

address jedrzej third face comments
@sKudum sKudum force-pushed the core/selectkeyManager branch from 5c21ca5 to 92477d8 Compare February 3, 2021 10:33
@sKudum sKudum requested a review from JKMarkowski February 3, 2021 10:33
JKMarkowski
JKMarkowski approved these changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@JKMarkowski JKMarkowski left a comment

Choose a reason for hiding this comment

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

LGTM

@sKudum sKudum merged commit 4e4c5fe into main Feb 3, 2021
@sKudum sKudum deleted the core/selectkeyManager branch February 3, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimamarksman dimamarksman dimamarksman approved these changes

@fkolar fkolar fkolar approved these changes

@JKMarkowski JKMarkowski JKMarkowski approved these changes

@KevinOkamoto KevinOkamoto Awaiting requested review from KevinOkamoto

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Sprint 56 - ariba
Development

Successfully merging this pull request may close these issues.

fix(core): use selectionmodel, key mamanagers cdk in select component
5 participants
@sKudum @dimamarksman @fkolar @JKMarkowski @manjunathanagaraj