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: assign positioning options per element (#5668) #5706

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

eitamyo
Copy link

@eitamyo eitamyo commented Mar 5, 2020

Fixes #5668
save positioning options per element to prevent overrides

PR Checklist

Before creating new PR, please take a look at checklist below to make sure that you've done everything that needs to be done before we can merge it.

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

Hi! I opened #5668 a couple of weeks ago, and this is my attempt at fixing it.
I noticed the options related to adaptive positioning were saved in a single property in the positioning service here . As a result, every time there were two components open, those options were overridden by the last one. So, my idea was to save the options separately for each component.
Any suggestions are welcome (should I add tests or demos?).

save positioning options per element to prevent
potential overrrides
@eitamyo eitamyo closed this Mar 9, 2020
@eitamyo eitamyo reopened this Mar 9, 2020
@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #5706 into development will decrease coverage by 0.3%.
The diff coverage is 83.33%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5706      +/-   ##
===============================================
- Coverage        73.92%   73.62%   -0.31%     
===============================================
  Files              323      323              
  Lines            11641    11639       -2     
  Branches          2477     2480       +3     
===============================================
- Hits              8606     8569      -37     
- Misses            1952     1968      +16     
- Partials          1083     1102      +19
Impacted Files Coverage Δ
...hemes/bs/bs-daterangepicker-container.component.ts 70.65% <ø> (-0.32%) ⬇️
...ker/themes/bs/bs-datepicker-container.component.ts 75.64% <ø> (-0.31%) ⬇️
src/typeahead/typeahead-container.component.ts 74.69% <0%> (-0.01%) ⬇️
src/popover/popover.directive.ts 73.73% <100%> (ø) ⬆️
src/datepicker/bs-datepicker.component.ts 68.25% <100%> (-1.59%) ⬇️
src/datepicker/bs-daterangepicker.component.ts 76.37% <100%> (ø) ⬆️
src/tooltip/tooltip.directive.ts 67.09% <75%> (-0.42%) ⬇️
src/typeahead/typeahead.directive.ts 79.92% <83.33%> (ø) ⬆️
src/chronos/i18n/pl.ts 70.27% <0%> (-13.52%) ⬇️
src/chronos/i18n/es-us.ts 53.33% <0%> (-13.34%) ⬇️
... and 17 more

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 d785e71...31b7a56. Read the comment docs.

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.

Tooltip inside popover overrides adaptive positioning
2 participants