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

Add modal overlay opening x,y offset settings. #1833

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

timodwhit
Copy link

Add the ability to offset the modal's overlay opening by an x,y value
for elements that are positioned uniquely on the screen. As an example:
This is useful for items that are placed inside an iframe that is in
a fixed position on the screen.

@RobbieTheWagner
Copy link
Member

@timodwhit looks like we have a failing test here. Mind taking a look?

@timodwhit
Copy link
Author

I do not mind taking a look, but I have never dealt with this test suite etc. So I might not be super fruitful in my endeavor.

@RobbieTheWagner
Copy link
Member

@monshan would you be able to look into this?

timodwhit and others added 3 commits June 16, 2022 10:36
Add the ability to offset the modal's overlay opening by an x,y value
for elements that are positioned uniquely on the screen. As an example:
This is useful for items that are placed inside an iframe that is in
a fixed position on the screen.
@monshan
Copy link
Contributor

monshan commented Jun 17, 2022

@rwwagner90 fixed the tests and rebased with most recent shepherd, should be good to go

@RobbieTheWagner
Copy link
Member

Awesome, thanks @monshan!

@RobbieTheWagner
Copy link
Member

@monshan would it be possible to add some tests for these new options? I don't think new tests were added here right?

@monshan
Copy link
Contributor

monshan commented Jun 18, 2022

@rwwagner90 no tests added by Tim but sure I could write some that cover the feature, my first thought is some cypress that asserts that it can offset in all 4 directions?

@RobbieTheWagner
Copy link
Member

@monshan yeah, that sounds perfect! Basically just testing at least a couple different values and making sure it moves.

@RobbieTheWagner
Copy link
Member

@monshan any updates on those tests? Would love to merge this soon 😄

@monshan
Copy link
Contributor

monshan commented Jun 22, 2022

@timodwhit
Testing the feature will produce the following:

  • Left: No offset, modal fills the window
  • Right: X-axis offset 25px, modal fills window except for whitespace according to the offset in the bottom left

Can you confirm that this is the desired behavior? Not that the modal offset will remain the same size as the window with adjusted positioning?

Screen Shot 2022-06-22 at 10 46 13 AM

@timodwhit
Copy link
Author

@monshan I'm sorry. I'm not sure I understand the question or the use case.

@monshan
Copy link
Contributor

monshan commented Jun 22, 2022

@timodwhit I just want to confirm that the shape of the SVG after offset is what you expected according to this PR?

@Steezli
Copy link
Contributor

Steezli commented Aug 16, 2022

@timodwhit @monshan is this issue resolved or are there still lingering questions regarding the tests for this feature?

@DerekJarvis
Copy link

@timodwhit @monshan is this issue resolved or are there still lingering questions regarding the tests for this feature?

I believe @monshan asked a question about the purpose of the PR and was awaiting a response. I found this PR because I was looking for this exact feature. I believe the purpose of this feature is to allow the overlay SVG that is generated to offset from the target element. This is useful in cases where the current CSS does not have margins around them and the overlay is very tight. It allows Shepherd to generate a better looking overlay without needing to rework the website's CSS.

@timodwhit
Copy link
Author

Sorry for the delay in the response and the long gap.

Yes, @monshan I believe your tests are correct.

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

5 participants