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

feat(modals): add support for nested modals (fix scroll) #1987

Merged
merged 3 commits into from
Jun 1, 2017
Merged

feat(modals): add support for nested modals (fix scroll) #1987

merged 3 commits into from
Jun 1, 2017

Conversation

H--o-l
Copy link
Contributor

@H--o-l H--o-l commented May 19, 2017

Twitter bootstrap (and so ngx-boostrap) doesn't officialy support nested modals.
You can however display nested modal but at least one issue appears:
At first modal you close, scroll events and scrollbar go back to the page, so
you can't scroll modals that are still displayed.

I propose to improve it with following modifications in modal component:

  • at show, check if 'modal open' class is already attached to document body and
    set isNested boolean accordingly
  • at hide, if nested, do not remove 'modal open' class and do not reset
    scrollbar.

Related to:
#896
#1691

@H--o-l
feat(modals): add support for nested modals (fix scroll)
c41e887 
Twitter bootstrap (and so ngx-boostrap) doesn't officialy support nested modals.
You can however display nested modal but at least one issue appears:
At first modal you close, scroll events and scrollbar go back to the page, so
you can't scroll modals that are still displayed.

I propose to improve it with following modifications in modal component:
- at show, check if 'modal open' class is already attached to document body and
  set `isNested` boolean accordingly
- at hide, if nested, do not remove 'modal open' class and do not reset
  scrollbar.

Related to:
#896
#1691
@codecov
Copy link

codecov bot commented May 19, 2017 •
edited
Loading

Codecov Report

Merging #1987 into development will increase coverage by 0.7%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           development    #1987     +/-   ##
==============================================
+ Coverage        85.93%   86.64%   +0.7%     
==============================================
  Files               78       77      -1     
  Lines             2098     2067     -31     
  Branches           273      270      -3     
==============================================
- Hits              1803     1791     -12     
+ Misses             202      183     -19     
  Partials            93       93
Impacted Files Coverage Δ
scripts/helpers.js
demo/src/app/app.component.ts
scripts/helpers.ts 100% <0%> (ø)

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 ead8d52...07105f3. Read the comment docs.

@adrienverge
Copy link
Contributor

Hi, what's the status on this?

It fixes the scroll problem on my project; I'd love to see it merged!

@H--o-l
Copy link
Contributor Author

H--o-l commented May 30, 2017

@IlyaSurmay Thanks for the documentation !

@YevheniiaMazur
Copy link

Сlicking on exit button on third modal window closes second modal window and makes third modal window inactive.
Scroll was fixed

@YevheniiaMazur
Copy link

Closing nested modals fixed. It looks good now

@valorkin valorkin merged commit 7ef989a into valor-software:development Jun 1, 2017
@valorkin
Copy link
Member

valorkin commented Jun 1, 2017

merged

@adrienverge adrienverge mentioned this pull request Jun 5, 2017
Closed
@IlyaSurmay IlyaSurmay mentioned this pull request Jun 23, 2017
Closed
@Meligy
Copy link

Meligy commented Jun 27, 2017 •
edited
Loading

I need to sit and give time to creating a proper bug report, but for now here's a quick note for anyone coming here via search like me:

This PR might have introduced a bug where calling hide() and show() multiple times synchronously, and some timing issue happens or something, causing the modal to think it's nested, and keeping the ClassName.OPEN class ('modal-open') applied to the <body> tag even after modal is closed, which causes the page to lose its scrollbars because of overflow:hidden; style.

The current workaround I added to my application is adding another directive with the same selector as the modal component, that disables animation on the component itself so that all the logic is run synchronously. Something like:

@Directive({
  selector: '[bsModal]'
})
export class BsModalOverrideDirective { 
  constructor(private modal: ModalComponent) { 
    modal.isAnimated = false;
  }
}

P.S.

Thanks heaps for all the great work on this awesome library.

@Meligy Meligy mentioned this pull request Jun 27, 2017
Closed
@valorkin
Copy link
Member

@Meligy I hope to drop ng v2 support soon and move to ng v4.2
where we will have AnimationBuilder
so things should go better

@valorkin
Copy link
Member

BTW we are almost done with ModalService
#2047

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
comp(modal)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@H--o-l @adrienverge @YevheniiaMazur @valorkin @Meligy @IlyaSurmay