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

Merged
merged 10 commits into from Jan 11, 2017

Projects

None yet

4 participants

@quanterion
Contributor
quanterion commented Nov 21, 2016 edited
@googlebot googlebot added the cla: yes label Nov 21, 2016
@jelbourn

@quanterion can you also add corresponding unit tests?

+
+/**
+ * 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.
@jelbourn
jelbourn Nov 29, 2016 Member

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

+ * It should be provided in the root component to ensure it is properly shared.
+ */
+@Injectable()
+export class FullscreenFriendlyOverlayContainer extends OverlayContainer {
@jelbourn
jelbourn Nov 29, 2016 Member

This class should go in its own file

+ }
+
+ private _getFullscreenElement(): Element {
+ return document.fullscreenElement ||
@jelbourn
jelbourn Nov 29, 2016 Member

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
Contributor
quanterion commented Nov 29, 2016 edited

@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
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
Contributor

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

@quanterion
Contributor

@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
Contributor

@jelbourn tests successfully passed after latest rebase

@quanterion
Contributor

@jelbourn any chance it can be reviewed?

@jelbourn
Member
jelbourn commented Jan 6, 2017

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

@jelbourn
Member
jelbourn commented Jan 6, 2017

@quanterion mostly looks good, just a few last comments

src/demo-app/demo-app/demo-app.ts
+
+ }
+
+ public toggleFullscreen() {
@jelbourn
jelbourn Jan 6, 2017 Member

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) {
@jelbourn
jelbourn Jan 6, 2017 Member

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

quanterion added some commits Nov 21, 2016
@quanterion quanterion feat(overlay): add fullscreen overlay class
5614e65
@quanterion quanterion fix lint error
d80f009
@quanterion quanterion move fs container to a new file and rename it
1162a88
@quanterion quanterion add e2e tests
d4f68a5
@quanterion quanterion fix tests
17bfd84
@quanterion quanterion rebased and fix comments
9b59f2d
@quanterion quanterion fix typings
842cbb7
@quanterion quanterion fix e2e tests
65bb323
@quanterion quanterion address comments
07e80c3
@quanterion quanterion fix tests
a24e2c4
@quanterion
Contributor

@jelbourn comments addressed and rebased

@jelbourn
Member

LGTM, thanks!

@tinayuangao tinayuangao merged commit 0640302 into angular:master Jan 11, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment