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.

Sign up for GitHub

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

Jump to bottom

refactor: switch to fullscreen.options #6054

Merged
merged 4 commits into from
Jun 17, 2019
Merged

refactor: switch to fullscreen.options #6054

merged 4 commits into from
Jun 17, 2019

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Jun 17, 2019

This allows us to then extend the fullscreen option with element and handler as necessary.

@gkatsev gkatsev mentioned this pull request Jun 17, 2019
Merged
7 tasks
brandonocasey
brandonocasey reviewed Jun 17, 2019
View reviewed changes
Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

Perhaps fullscreen.requestOptions or fullscreen.apiOptions would make more sense? If not I think the code looks good.

@gkatsev gkatsev mentioned this pull request Jun 17, 2019
Merged
@gkatsev
Copy link
Member Author

gkatsev commented Jun 17, 2019

Updated to fix the memory leak. This actually would only be an issue with tests.

I kind of like the simplicity of fullscreen.options.

brandonocasey
brandonocasey approved these changes Jun 17, 2019
View reviewed changes
Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

Good to know the memory leak test actually works! Even though it only caught a test leak

@gkatsev
Copy link
Member Author

gkatsev commented Jun 17, 2019

Yeah, it's just how the test was created. Though, a bit surprised that we didn't notice it when the PR was made? Maybe the test was added after?

@gkatsev gkatsev merged commit 2977d52 into master Jun 17, 2019
@gkatsev gkatsev deleted the fullscreen-options branch June 17, 2019 20:51
@gkatsev gkatsev mentioned this pull request Jun 25, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonocasey brandonocasey brandonocasey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gkatsev @brandonocasey