Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(modal): ensure correct index is set for new modals #5733

Closed
wants to merge 5 commits into from
Closed

fix(modal): ensure correct index is set for new modals #5733

wants to merge 5 commits into from

Conversation

josetaira
Copy link
Contributor

Fix how the index for the modal and the backdrop are set, depending on
what index value the previous top modal has.

Closes #5670

fix(modal): ensure correct index is set for new modals
1a436de 
Fix how the index for the modal and the backdrop are set, depending on
what index value the previous top modal has.

Closes #5670
wesleycho
wesleycho reviewed Apr 4, 2016
View reviewed changes
src/modal/modal.js

// If any backdrop exist, ensure that it's index is always
// right below the top modal
if(topBackdropIndex > -1 && topBackdropIndex < topModalIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space after if

@wesleycho
Copy link
Contributor

Can you add a unit test for this?

@josetaira
fix(modal): fix formatting comments
40f3df9 
Fix the formatting comments of wesleycho on recent commit for modal fix

Closes #5670
@josetaira
Copy link
Contributor Author

For the tests, no idea how grunt works, but I guess I can learn it. Do you have any templates or guidelines on how tests should be made?

@icfantv
Copy link
Contributor

icfantv commented Apr 4, 2016

@josetaira, you don't need to learn grunt, you need to look at the model.spec.js file and add your tests to that file and they'll run as part of the build. there should be enough examples in there (and our other test files) to figure it out.

if you want to run a specific test or test suite, simply prefix the it or describe with an f (i.e. fit or fdescribe) and only that test will be run. our linter will reject this as invalid syntax so you can run karma start to run the karma tests independently.

fix(modal): add code to adjust index for modal
8c55c6e 
Add code that adjusts the angularDomEl index properly based on
topModalIndex. Just forgot to add this to the initial commit.

Closes #5670
@josetaira
Copy link
Contributor Author

@icfantv Thanks for the info. Working on the unit tests now. Seems I did have to use grunt to create some templates (run grunt default before any tests). I'll add a commit for the test later on.

@icfantv
Copy link
Contributor

icfantv commented Apr 5, 2016

@josetaira good deal.

Pem Taira added 2 commits April 5, 2016 05:46
fix(modals): add unit test and add missing line
e08b5f5 
Add unit test to check proper index is set. Add the missing line that sets
the value of previousTopOpenedModal.

Closes #5670
fix(modal): fix CI failed checks for parseInt
4ca026a 
Fix the missing radix issue for parseInt used in tests

Closes #5670
@josetaira
Copy link
Contributor Author

@icfantv @wesleycho tests committed.

@wesleycho
Copy link
Contributor

This LGTM - thanks for the effort!

@wesleycho wesleycho added this to the 1.3.1 milestone Apr 5, 2016
@wesleycho wesleycho closed this in a08ad70 Apr 5, 2016
@josetaira josetaira deleted the fix-multiple-modal-index branch June 16, 2016 10:48
@josetaira josetaira restored the fix-multiple-modal-index branch June 16, 2016 10:49
@josetaira josetaira deleted the fix-multiple-modal-index branch June 16, 2016 10:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
component: modal
Projects
None yet
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@josetaira @wesleycho @icfantv