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

NG_SELECT_DEFAULT_CONFIG DI fix #863

Merged
merged 8 commits into from
Oct 19, 2018
Merged

NG_SELECT_DEFAULT_CONFIG DI fix #863

merged 8 commits into from
Oct 19, 2018

Conversation

atakchidi
Copy link
Contributor

This pr fixes issue #862

Now config is working as expected.

  • It has default implementation, user desides how and where to provide it, so no more unexpected behaviour.
  • Config parameters could be partly provided, as docs and interface suggests, defaults for unset params will still persist

@coveralls
Copy link

coveralls commented Oct 11, 2018 •
edited

Coverage Status

Coverage decreased (-0.07%) to 94.858% when pulling 9bf53cc on atakchidi:default-config-fix into fe9d990 on ng-select:master.

@anjmao anjmao self-requested a review October 11, 2018 13:42
anjmao
anjmao requested changes Oct 11, 2018
View reviewed changes
Copy link
Member

@anjmao anjmao left a comment •
edited

Choose a reason for hiding this comment

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

Hi @atakchidi,

Thanks for contributing, indeed currect configuration DI is not correct. It would be great to have the same Config injection as in ng-boostrap.

src/ng-select/config.ts Outdated Show resolved Hide resolved
@atakchidi
Copy link
Contributor Author

Ok, this makes sence. I'll rewrite this somewhere around next week when have time.

@jiminikiz
Copy link

Looking forward to this PR!

@atakchidi
Copy link
Contributor Author

@anjmao @varnastadeus please review last changes

@varnastadeus
Copy link
Member

@atakchidi I merged conflicts through github, but can you add openOnEnter to config with true as default?

@atakchidi
Copy link
Contributor Author

@varnastadeus yes sure, here u go.

@varnastadeus varnastadeus merged commit 8bea572 into ng-select:master Oct 19, 2018
@ivanpadavan ivanpadavan mentioned this pull request Nov 7, 2018
Closed
NickMele pushed a commit to NickMele/ng-select that referenced this pull request Apr 12, 2019
@atakchidi @varnastadeus
fix(NgSelectConfig): use class as config provider
96c94f5 
fixes ng-select#863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anjmao anjmao anjmao requested changes

@varnastadeus varnastadeus varnastadeus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NG_SELECT_DEFAULT_CONFIG is wrongly provided (should be global)
5 participants
@atakchidi @coveralls @jiminikiz @varnastadeus @anjmao