-
-
Notifications
You must be signed in to change notification settings - Fork 647
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
Separate modal overlay opening element option #1837
base: main
Are you sure you want to change the base?
Separate modal overlay opening element option #1837
Conversation
Just tried the failing tests locally and they're passing for me. What's the best way to get more info out of CI? I couldn't find a way to get at any specific error logs or the screenshots. |
@wbobeirne you can see the failing tests by clicking details next to them. Seems like the test for |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@wbobeirne would you be able to rebase this with master? We've made some changes, so perhaps tests will work again. |
@rwwagner90 I looked into this but @wbobeirne 's fork is password protected so I can't push my rebase to his branch, lmk your thoughts if its worth cloning and opening a new PR |
@monshan if you use the GitHub desktop app, you can pull down this PR instead of a branch. That might be a good way to pull it down, rebase, and push it back up to this PR. |
…ement to draw overlay around Added re-usable `parseElementOrSelector` function for new arg and `attachTo.element` Rebases with master
Also added getOpeningProperties function to modal class to easily verify in testing
824acae
to
3b1f4d5
Compare
@monshan looks like we have some failing tests here still. How are things looking? |
@rwwagner90 I thought the scope of this one would just be to rebase it which went through but still not passing. If I remember right its because this change conflicts with the Shepherd 10 upgrade because now the |
Ah yes, that sounds correct. @wbobeirne could you please refactor this to be compliant with the new |
@EmNicholson93 I think this needs some refactoring to be compliant with the new |
Closes #1836
modalOverlayOpeningElement
option for specifying anHTMLElement
or string selector for what the modal should targetgetOpeningProperties
method to modal so I could confirm the dimensions matched up with the desired element. Let me know if you'd rather this test be written in a different way that didn't require changing the API of modal.general
to reuse aparseElementOrSelector
function for the new option andattachTo.element
parseElementOrSelector
function