-
-
Notifications
You must be signed in to change notification settings - Fork 649
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
Added support to open Shepherd inside <dialog> elements with the correct offset. #191
Conversation
document.body.appendChild(this.el); | ||
|
||
// Handling open dialog elements. | ||
if (document.querySelector('dialog[open]')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with dialog elements, but wouldn't it be possible someone could have one open, but still want to append to the body instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally open dialog elements having the main focus and getting closed after an specific action is performed like window.confirm() or window.promt().
Example: https://codepen.io/Garbee/pen/EPoaMj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but it is possible you would have an open dialog and still want to not attach your steps to it. I think a better solution to this problem might be to allow passing a context and default to document.body
when one is not passed. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the general scenarios:
- Main window has steps, Dialog has steps.
- Main window has no steps, Dialog has steps.
- Main window has steps, Dialog has no steps.
All three scenarios are covered in this case:
- Steps are attached to body. Dialog gets open. Dialog steps are attached to the Dialog.
- Dialog gets open. Dialog steps are attached to the Dialog.
- Steps are attached to body.
Edge Case:
The dialog gets open before the main window adding the steps.
This should theoretical never happen, because such dialog normally only shown on user interaction or after the main content is rendered.
Did I miss anything?
@MarkusBordihn perhaps I was not being clear, but I have opened a PR, in which you can pass That way, we can support rendering in any element, not just handle dialogs, and this gives greater flexibility. Please check it out and let me know if it will work for you #192. |
@rwwagner90 |
@MarkusBordihn the same would also be true of this PR, right? Or did I mess something up? |
@rwwagner90 |
@MarkusBordihn popper.js supports offsets already. Perhaps try changing styles or offsets via |
@rwwagner90 I just tested the latest version and it works well with native dialogs if you define the defaultStepOptions together with tippyOptions for e.g.:
Thanks a lot for improving Shepherd and keep it up to date. ;) |
You're welcome @MarkusBordihn! Glad it is working well for you 😃 |
Fixed #157 with Popper.js implementation.