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

feat(overlay): add fullscreen-enabled overlay class #1949

Merged
merged 10 commits into from
Jan 11, 2017

Conversation

quanterion
Copy link
Contributor

@quanterion quanterion commented Nov 21, 2016

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 21, 2016
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

@quanterion can you also add corresponding unit tests?

* It should be provided in the root component to ensure it is properly shared.
*/
@Injectable()
export class FullscreenFriendlyOverlayContainer extends OverlayContainer {
Copy link
Member

Choose a reason for hiding this comment

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

This class should go in its own file


/**
* The OverlayContainer is the container in which all overlays will load.
* It should be provided in the root component to ensure it is properly shared.
Copy link
Member

Choose a reason for hiding this comment

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

Needs a class description that explains what this class is for specifically.

}

private _getFullscreenElement(): Element {
return document.fullscreenElement ||
Copy link
Member

Choose a reason for hiding this comment

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

Need a comment here like

// When the page is put into fullscreen mode, a specific element is specified. 
// Only that element and its children are visible when in fullscreen mode. 

@quanterion
Copy link
Contributor Author

quanterion commented Nov 29, 2016

@jelbourn, comments addresed, also renamed container to FullscreenOverlayContainer as it sounds less verbose, can revert back to old name if it is not appropriate.

I tried do add unit-tests like this:

it('should move container into a fullscreen element', () => {
    let c = overlayContainer.getContainerElement();    
    expect(c.parentElement).toBe(document.body);
    let fullscreenPane = document.createElement('div');
    document.body.appendChild(fullscreenPane);
    activateFullscreen(fullscreenPane);
    expect(c.parentElement).toBe(fullscreenPane);
    exitFullscreen();
    expect(c.parentElement).toBe(document.body);
    fullscreenPane.remove();
  });

But they are failing because Chrome says: Failed to execute 'requestFullScreen' on 'Element': API can only be initiated by a user gesture.

@jelbourn
Copy link
Member

@quanterion sounds like we'd need to use an e2e test for this behavior, then, since webdriver acts as user interaction. Can you add an e2e test for this?

@quanterion
Copy link
Contributor Author

@jelbourn added e2e tests and rebased on top of the master

@quanterion quanterion force-pushed the fullscreen_overlay_class branch 2 times, most recently from 9ee0af0 to c4909bd Compare December 20, 2016 19:16
@quanterion
Copy link
Contributor Author

@jelbourn rebased on top of the latest sdk changes and tests starts failing and I can't understand why, the errors seems to be unrelated to my changes

@quanterion
Copy link
Contributor Author

@jelbourn tests successfully passed after latest rebase

@quanterion
Copy link
Contributor Author

@jelbourn any chance it can be reviewed?

@jelbourn
Copy link
Member

jelbourn commented Jan 6, 2017

@quanterion working on getting through the backlog of PRs in the wake of beta and holidays now

@jelbourn
Copy link
Member

jelbourn commented Jan 6, 2017

@quanterion mostly looks good, just a few last comments


}

public toggleFullscreen() {
Copy link
Member

Choose a reason for hiding this comment

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

We typically omit the public access modifier since it's the default

* returns true if it has tried to toggle fullscreen mode
* but provides no guarantees whether it really happened
*/
toggleFullscreen(element: HTMLElement) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to omit this function from here, since it falls outside of the responsibility of the OverlayContainer

@quanterion
Copy link
Contributor Author

@jelbourn comments addressed and rebased

@jelbourn
Copy link
Member

LGTM, thanks!

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 10, 2017
@tinayuangao tinayuangao merged commit 0640302 into angular:master Jan 11, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants