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(typeahead): allow custom class on window for modal issue #3947

Merged
merged 2 commits into from
Mar 23, 2021

Conversation

eliprand
Copy link
Contributor

This PR enables providing one or more custom classes to the typeahead window. It's intended to help solve #3778 as per my comment.

I modeled the behavior and implementation after windowClass from NgbModal.The main intent here is allowing users to provide a custom z-index value along with the container="body" option for their specific needs. But ultimately, it can be used to override any attributes on the ngb-typeahead-window.

I did not feel we needed to update the demo, but I would be happy to do so if needed.

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md and DEVELOPER.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

Eric Liprandi added 2 commits January 14, 2021 17:50
enable basic support for windowClass on typeahead
provide simple flow from typeahead directive to typeahead window
@eliprand eliprand marked this pull request as ready for review January 15, 2021 01:06
@eliprand eliprand closed this Jan 15, 2021
@eliprand eliprand reopened this Jan 15, 2021
@codecov-io
Copy link

Codecov Report

Merging #3947 (8df7971) into master (dce3886) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3947   +/-   ##
=======================================
  Coverage   90.70%   90.70%           
=======================================
  Files         110      110           
  Lines        3185     3186    +1     
  Branches      609      609           
=======================================
+ Hits         2889     2890    +1     
  Misses        204      204           
  Partials       92       92           
Flag Coverage Δ
e2e 49.23% <100.00%> (+0.01%) ⬆️
unit 87.41% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/typeahead/typeahead-window.ts 100.00% <ø> (ø)
src/typeahead/typeahead.ts 96.03% <100.00%> (+0.03%) ⬆️

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 dce3886...8df7971. Read the comment docs.

@eliprand eliprand changed the title fix for typeahead in modal feate(typeahead): allow custom class on window for modal issue Jan 15, 2021
@eliprand eliprand changed the title feate(typeahead): allow custom class on window for modal issue feat:(typeahead): allow custom class on window for modal issue Jan 15, 2021
@eliprand eliprand changed the title feat:(typeahead): allow custom class on window for modal issue feat(typeahead): allow custom class on window for modal issue Jan 15, 2021
@maxokorokov maxokorokov added this to the 9.1.0 milestone Mar 23, 2021
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@maxokorokov maxokorokov merged commit e7457f4 into ng-bootstrap:master Mar 23, 2021
@eliprand
Copy link
Contributor Author

@maxokorokov thanks for merging. FYI, can I/we update the inline docs for the typeahead.ts changes I made? I was optimistic and say @since 8.1.0 but it looks like we're going out with 9.1.0. Can I update this code here? or is it too late?

@maxokorokov
Copy link
Member

@eliprand we're usually doing it before the feature release, don't worry
I've also renamed windowClass to popupClass later, because publicly (in docs) everywhere we're referring to it as typeahead popup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants