Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix(popup-stack): Add support for the fullscreen API #1403
fix(popup-stack): Add support for the fullscreen API #1403
Changes from 2 commits
ae9a581
841fe82
6e0b093
4f2b0ae
b14ed50
3409105
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
screenfull was chosen because it is as lightweight as possible and already used by some teams.
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.
Optional. Falls back to
document.body
if not defined.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.
why are we removing the typing here for
_adapter
anditems
?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.
stack
is already declared as typeStack
which includes typing for_adapter
, so this casting is not necessary.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.
Maybe too early to consider this, but is the plan to eventually deprecate the singular stack? If so, we should note this is only here for backwards compatibility as well
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.
That's possible, but a single stack needs to be created as the base stack and that needs to be shared. It could be that the first import of
PopupStack
is a version before this change and this variable would still exist. It could be that this code is removed and an old version that uses this still adds the global variable. It is important that all instances of this file use the same reference.It is possible to deprecate, but that would take a lot of thought and testing. I have no plans to remove it at the moment.
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.
This is the new variable added to
window
. This can be observed to see what stack we're currently in. Adapters should still manipulate this object.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.
All the
|| document.body
is backwards compatible code. If this version ofPopupStack
isn't the first loaded, thecontainer
would not be defined. ThePopupStack
tries to register a global sharedwindow.workday.__popupStack
variable. This code cannot be removed.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.
This should have always been
true
. Since we manipulatewindow
outside of a function call, this is required to support tree shaking. Proper tree shaking would break previous versions ofPopupStack
. I'm surprised we haven't received any bug reports.