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

fix(core): export MdOptionSelectionChange #4285

Merged
merged 1 commit into from
Apr 29, 2017
Merged

Conversation

epelc
Copy link
Contributor

@epelc epelc commented Apr 26, 2017

This exports MdOptionSelectionChange in @angular/material. It makes it easier to import if you are making a custom component which uses mdOption and needs to handle selection changes itself(ie a custom search bar).

I couldn't find any guidelines on when or why certain things are exported but not exposed in the main @angular/material package.

Feel free to close if we should just import from @angular/material/core/option instead. It seems importing from @angular/material/core/option doesn't work even though there are type files.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 26, 2017
@epelc
Copy link
Contributor Author

epelc commented Apr 26, 2017

@jelbourn Any thoughts on this? I also just found that ActiveDescendantKeyManager isn't exported from core.ts but it is from it's file.

Are these exports considered internal if they aren't exported again in core.ts? If so, would you be ok with exporting ActiveDescendantKeyManager and MdOptionSelectionChange? They are very useful/required if you need to make a more advanced autocomplete/search bar.

My use case is our search bar which is a mix of auto complete and "Chip" style tag/field suggestions. Kind of similar to gitlab's tag suggestions in their issue search.

epelc added a commit to epelc/material2-built that referenced this pull request Apr 26, 2017
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Apr 27, 2017
@epelc
Copy link
Contributor Author

epelc commented Apr 27, 2017

@crisbeto Can I export ActiveDescendantKeyManager too?

@crisbeto
Copy link
Member

crisbeto commented Apr 27, 2017

I'll double check whether it's something we'd want to export. It might be more appropriate as a part of #4290 though.

@epelc
Copy link
Contributor Author

epelc commented Apr 27, 2017

@crisbeto It works if you export it(idk if that's what your checking). I'll put it in #4290 if you'd like.

@mmalerba mmalerba merged commit af978cd into angular:master Apr 29, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants