feat(aio): force live-example to embedded-style on narrow screen by wardbell · Pull Request #15887 · angular/angular

wardbell · 2017-04-11T01:20:49Z

Updated

@IgorMinar asked that "mobile" screen constraint be determined by viewport size, not user-agent.

After further experimentation, I determined that, while the regular style plunker is useless on narrow screens, the embedded-style plunker is functional which means that we can show plunkers on mobile devices.

Therefore, this PR's 2nd commit changes the logic. It forces plunkers to embedded style on narrow (1000px) screens. It allows download of sample zips (I suppose you might want to email it from your phone).

It removes the user-agent test for mobile devices from the first commit. It keeps the refactoring of window resize in the DeviceService because that check is needed in two places and because it seems like a favorable abstraction for future use.

@sjtrimble CSS changes to control the embedded image width on narrow screens is more valid than ever.

Original

Cannot run plunkers on mobile devices nor does it make sense to download code sample to such devices. Therefore, should detect when on mobile and tell user that the live example is not available.

PR adds DeviceService to detect mobile device. The check does not consider a tablet (e.g., iPad) a mobile device because I believe that plunkers should display fine on them. While it is foolish to download the code to a tablet, we do not prevent it.

DeviceService also listen for window resize events. Moved window resize detection from AppComponent to DeviceService.displayWidth observable so others can hear about it from a shared, device-focused service.

I thought this would be important for addressing the no-LiveExample-on-mobile requirement. It turns out it wasn't needed but I kept it there because it feels like AppComponent should delegate this responsibility to a service that knows about the device.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

mary-poppins · 2017-04-11T01:29:07Z

The angular.io preview for 9adc212 is available here.

gkalpak

No tests for DeviceService ? 😱

gkalpak · 2017-04-11T13:53:41Z

Do you need all previous values? I would expect we only care about the last one.

I frankly don't know how to test DeviceService itself because it interacts directly with the browser environment and I have no idea how to simulate that. I'm open to ideas.

Note that we never tested this interaction before my refactor. Thanks to the DeviceService, we can simulate the effects of different devices on the application code w/o actually changing devices.

My bad. Should be new ReplaySubject<number>(1); . Fixing.

gkalpak · 2017-04-11T15:01:37Z

I am not thrilled to access window directly here, but not a blocking issue 🌊

Do you have a suggestion?

In another PR, I used a Global provider so that I can more easily/cleanly mock it in tests. But maybe I am overthinking it. I am too used to AngularJS' $window 😁

gkalpak · 2017-04-11T15:12:07Z

We should have a test for both embedded and non-embedded plnkrs.

gkalpak · 2017-04-11T15:12:22Z

Can we please move all the helpers in one place?

mary-poppins · 2017-04-11T21:44:01Z

The angular.io preview for 8ff736f is available here.

gkalpak · 2017-04-12T11:56:52Z

Wow! OK, I will trust StackOverflow on this one 😁

why do we use useragent string for this? it's the viewport resolution that matters not if it's a mobile device. Can we just use that instead?

@IgorMinar I thought about that. It is the device and not the viewport that determines whether the link should show.

On desktop I might have the browser window narrow. I can always expand it to allow the plunker to play. I can also download the code. Neither of these options are available on mobile.

Are you suggesting that we change the logic so that plunkers/download are only available when the viewport is a certain width? I'm not sure what that width is but we could experiment. The download block, based on width, would be silly but we could get over it.

I await your decision.

Please use viewport size. Only developers will see the site in narrow mode on desktop.

gkalpak

A couple of nitpicks.
Regarding the DeviceService tests: Not a blocker (especially since we didn't test this before), but there is some logic in DeviceService that could be tested.
(An injectable global/window would indeed come in handy here.)

gkalpak · 2017-04-12T12:02:00Z

Unnecessary ;

gkalpak · 2017-04-12T12:02:33Z

Unnecessary space between isMobileCheck and () 😁

mary-poppins · 2017-04-12T20:29:18Z

The angular.io preview for 9d304ce is available here.

mary-poppins · 2017-04-12T23:24:17Z

The angular.io preview for fba26fb is available here.

wardbell · 2017-04-12T23:24:20Z

Reworked the logic to depend on viewport width and eliminate user-agent check. See revised initial PR comment which explains everything. Adjusted PR title accordingly.

gkalpak

Can you write up the rules of when each plnkr style is applied (and whether it should be opened in a new tab etc). I am pretty confused atm 😕

gkalpak · 2017-04-14T08:19:17Z

Isn't this supposed to be flat-style (with a dash)?

It can be any of the aliased choices - "flatstyle", "flat-style", or "flatStyle" - known to appear in the docs. I didn't feel it was worth testing them all. But I will pick the first mentioned, "flat-style"

gkalpak · 2017-04-14T08:21:32Z

Nit: this.attrs.name --> attrs.name 😃

gkalpak · 2017-04-14T09:50:45Z

"even when"?

Oops. Copy/paste error. Just "when ... requested"

The "rules" are described in the documentation at the top.

gkalpak · 2017-04-14T09:57:05Z

We are calling this onDestroy in ApiService and SwUpdatesService. Let's be consistent 😁

gkalpak · 2017-04-14T10:00:50Z

In this block, we know that width > this.narrowWidth.

…ilable Adds DeviceService to detect mobile device and listen for window resize events.

Regular plunker is unusable on narrow screen Refactors LiveExampleComponent and adds tests. Refactor width detection to `DeviceService` because need to know width change in 2 places. Keep “disable” option add in earlier spikes because simple and potentially useful in future.

mary-poppins · 2017-04-15T06:20:08Z

The angular.io preview for aced5e0 is available here.

mary-poppins · 2017-04-15T06:23:46Z

The angular.io preview for d00565b is available here.

angular-automatic-lock-bot · 2019-09-11T09:20:36Z

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.}

wardbell added the comp: aio label Apr 11, 2017

wardbell self-assigned this Apr 11, 2017

googlebot added the cla: yes label Apr 11, 2017

gkalpak suggested changes Apr 11, 2017

View reviewed changes

wardbell force-pushed the aio-no-live-example-on-mobile branch from 9adc212 to 8ff736f Compare April 11, 2017 21:30

gkalpak mentioned this pull request Apr 12, 2017

feat(aio): marketing, filetree and cleanup #15911

Merged

3 tasks

gkalpak reviewed Apr 12, 2017

View reviewed changes

gkalpak approved these changes Apr 12, 2017

View reviewed changes

gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 12, 2017

wardbell force-pushed the aio-no-live-example-on-mobile branch from 8ff736f to 9d304ce Compare April 12, 2017 20:18

wardbell force-pushed the aio-no-live-example-on-mobile branch from b493409 to fba26fb Compare April 12, 2017 23:14

wardbell changed the title ~~feat(aio): detect mobile device; tell user live-example is not available~~ feat(aio): force live-example to embedded-style on narrow screen Apr 12, 2017

gkalpak suggested changes Apr 14, 2017

View reviewed changes

feat(aio): detect mobile device and tell user live-example is not ava…

0b878d5

…ilable Adds DeviceService to detect mobile device and listen for window resize events.

wardbell added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 15, 2017

wardbell force-pushed the aio-no-live-example-on-mobile branch from fba26fb to aced5e0 Compare April 15, 2017 06:10

wardbell force-pushed the aio-no-live-example-on-mobile branch from aced5e0 to d00565b Compare April 15, 2017 06:14

gkalpak approved these changes Apr 15, 2017

View reviewed changes

wardbell added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 15, 2017

petebacondarwin merged commit 9a50844 into angular:master Apr 16, 2017

wardbell mentioned this pull request Apr 19, 2017

fix(aio): revert resize to HostListener; delete device svc #16143

Closed

3 tasks

wardbell deleted the aio-no-live-example-on-mobile branch April 19, 2017 18:17

petebacondarwin mentioned this pull request Apr 20, 2017

aio: fix live-server styling #15935

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 11, 2019

Conversation

wardbell commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Updated

Original

Uh oh!

mary-poppins commented Apr 11, 2017

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Apr 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardbell Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Apr 12, 2017

Uh oh!

mary-poppins commented Apr 12, 2017

Uh oh!

wardbell commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardbell Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardbell commented Apr 11, 2017 •

edited

Loading

wardbell Apr 12, 2017 •

edited

Loading

wardbell commented Apr 12, 2017 •

edited

Loading

wardbell Apr 15, 2017 •

edited

Loading