feat(aio): add survey link #21371

petebacondarwin · 2018-01-07T21:41:46Z

petebacondarwin · 2018-01-07T21:43:38Z

aio/src/app/app.component.html

+    </a>
+    <aio-top-menu *ngIf="isSideBySide" [nodes]="topMenuNodes"></aio-top-menu>
+    <aio-search-box class="search-container" #searchBox (onSearch)="doSearch($event)" (onFocus)="doSearch($event)"></aio-search-box>
+  </mat-toolbar-row>


The changes between this lines 18 and 28 are just because I had to move the toolbar content into a toolbar-row.

petebacondarwin · 2018-01-07T21:44:48Z

aio/src/app/app.component.ts

+        style({height: '*'}),
+        animate(250, style({height: 0}))
+      ])
+    ])


If we add the :enter transition to this animation then we could also use it for displaying the search results more smoothly.

petebacondarwin · 2018-01-07T21:46:17Z

aio/src/app/app.component.ts

+  private readonly SURVEY_EXPIRATION_DATE = new Date(2018, 1, 22);
+  private get localStorage() { return this.window.localStorage; }
+  showSurvey = !this.localStorage.getItem(this.SURVEY_KEY) && this.SURVEY_EXPIRATION_DATE > this.currentDate;
+


I worried that this is rather hard coded for this "generic" shell application but in this case, I felt that it was a temporary and rare feature that didn't warrant building the infrastructure t make this dynamically data driven.

I'm actually expect that we'll use this feature a lot, so it would be better if we moved this to the proposed "NotificationComponent".

petebacondarwin · 2018-01-07T21:47:27Z

aio/src/app/app.module.ts

+        '<path d="M0 0h24v24H0z" fill="none"/>' +
+      '</svg>'
+    },
+    multi: true


I hardcoded the two icons that are needed by the survey link component so that there is no flash of unwanted text. This will of course make the initial JS file slightly larger.

:-( we should find a way to deal with this better in the future

We could try using HTTP push to preload these files so that we don't need to include them in the source...

petebacondarwin · 2018-01-07T21:48:15Z

aio/src/styles/1-layouts/_top-menu.scss

@@ -10,7 +10,6 @@ mat-toolbar.mat-toolbar {
  right: 0;
  left: 0;
  z-index: 10;
-  padding: 0 16px 0 0;


Now we are using toolbar rows this messed up the alignment.

petebacondarwin · 2018-01-07T21:49:06Z

aio/src/styles/2-modules/_survey.scss

+      padding-top: 10px;
+      @media (max-width: 400px) {
+        display: none;
+      }


If the screen gets super small (>400px) then we also hide the survey-icon, otherwise there is not enough space to display the text and the close button.

mary-poppins · 2018-01-07T22:22:42Z

You can preview 0f3b280 at https://pr21371-0f3b280.ngbuilds.io/.

IgorMinar

the rest looks great. thanks for speedy turnaround!

IgorMinar · 2018-01-08T04:50:41Z

aio/scripts/_payload-limits.json

        "polyfills": 11858
      },
      "uncompressed": {
        "inline": 1558,
-        "main": 456432,
+        "main": 462030,


how did this small feature add 6kb? was it because of some material design component?

can you please check the sourcemap explorer to see if you can identify where all these kbs are coming from? your own code can't weigh more than 1kb.

IgorMinar · 2018-01-08T04:55:23Z

aio/src/app/app.component.ts

+  private readonly SURVEY_EXPIRATION_DATE = new Date(2018, 1, 22);
+  private get localStorage() { return this.window.localStorage; }
+  showSurvey = !this.localStorage.getItem(this.SURVEY_KEY) && this.SURVEY_EXPIRATION_DATE > this.currentDate;
+


I'm actually expect that we'll use this feature a lot, so it would be better if we moved this to the proposed "NotificationComponent".

IgorMinar · 2018-01-08T04:58:21Z

aio/src/app/layout/survey-link/survey-link.component.html

+    <mat-icon class="survey-icon" [svgIcon]="icon" [attr.aria-label]="iconText"></mat-icon>
+  </a>
+  <a href="{{surveyUrl}}" (click)="hideSurvey.next()" class="survey-link">{{message}}</a>
+  <a href="{{surveyUrl}}" (click)="hideSurvey.next()" class="survey-button">{{buttonText}}</a>


I originally thought that only dismissing the notification via the close button would permanently hide it. But I guess you are right that even on android/ios if you click on a notification the notification disappears. So let's keep it this way.

IgorMinar · 2018-01-08T04:58:51Z

aio/src/app/layout/survey-link/survey-link.component.spec.ts

+  beforeEach(() => {
+    TestBed.configureTestingModule({
+      declarations: [ SurveyLinkComponent ],
+      schemas: [NO_ERRORS_SCHEMA]


why NO_ERRORS_SCHEMA?

So we don't have to load the Material components just for the unit testing

IgorMinar · 2018-01-08T05:14:21Z

aio/src/app/app.component.html

-  <aio-top-menu *ngIf="isSideBySide" [nodes]="topMenuNodes"></aio-top-menu>
-  <aio-search-box class="search-container" #searchBox (onSearch)="doSearch($event)" (onFocus)="doSearch($event)"></aio-search-box>
+  <mat-toolbar-row class="survey-container" [@accordion]="showSurvey" *ngIf="showSurvey">
+    <aio-survey-link


uh. oh. I'm sorry that I wasn't clear enough in the specification. This should have been just a generic NotificationComponent that takes campaign ID (used for local storage identifier), expiration date, an action url, and a html content to be reprojected.

<aio-notification campaignId="someId" expiration="2018-01-20" icon="insert_comment" /* optional, defaults to insert_comment */ labels="Go to survey" href="https://bit.ly/angular-survey-2018"> Help Angular by taking a <b>1 minute survey</b>! </aio-notification>

The component can have a host listener for the click events. If a user clicks on the host element, the component will store info about the notification being dismissed and then it will navigate to the href.

IgorMinar · 2018-01-08T16:03:57Z

Aha!

…

On Mon, Jan 8, 2018, 6:35 AM Pete Bacon Darwin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In aio/src/app/layout/survey-link/survey-link.component.spec.ts <#21371 (comment)>: > @@ -0,0 +1,65 @@ +import { NO_ERRORS_SCHEMA } from ***@***.***/core'; +import { ComponentFixture, TestBed } from ***@***.***/core/testing'; +import { By } from ***@***.***/platform-browser'; +import { SurveyLinkComponent } from './survey-link.component'; + +describe('TopMenuComponent', () => { + let component: SurveyLinkComponent; + let fixture: ComponentFixture<SurveyLinkComponent>; + + beforeEach(() => { + TestBed.configureTestingModule({ + declarations: [ SurveyLinkComponent ], + schemas: [NO_ERRORS_SCHEMA] So we don't have to load the Material components just for the unit testing — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#21371 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANM6HSpiYkGIUOJZVhNmqUAkS7cIBd6ks5tIieYgaJpZM4RVy_a> .

mary-poppins · 2018-01-08T17:36:38Z

You can preview 9629552 at https://pr21371-9629552.ngbuilds.io/.

mary-poppins · 2018-01-08T17:51:00Z

You can preview 835a0f0 at https://pr21371-835a0f0.ngbuilds.io/.

mary-poppins · 2018-01-08T18:35:17Z

You can preview 40a71b8 at https://pr21371-40a71b8.ngbuilds.io/.

petebacondarwin · 2018-01-08T18:57:59Z

Made all the updates and investigated the change to the size. Locally I could only see in source-map-explorer about 3k of size increase due to the new functionality. Not quite sure where the 5k in the CI build is coming from.

StephenFluin · 2018-01-08T19:09:19Z

Also, if we wanted to put an end date it now we can. We are planning to run the survey through Jan 19th. This can be added separately / or just removed when the time is right in a different PR if you prefer.

gkalpak

For reference:
As discussed offline, the marketing pages have a white top offset when the notification is shown.

mary-poppins · 2018-01-11T13:29:33Z

You can preview b0eaa08 at https://pr21371-b0eaa08.ngbuilds.io/.

mary-poppins · 2018-01-11T14:04:47Z

You can preview dbd2e3c at https://pr21371-dbd2e3c.ngbuilds.io/.

mary-poppins · 2018-01-11T14:16:45Z

You can preview 22c0489 at https://pr21371-22c0489.ngbuilds.io/.

alexeagle · 2018-01-11T14:46:15Z

@gkalpak okay to merge now?

gkalpak

Oh, I didn't realize the merge label was applied. (I would have removed it.)
Everything looks great now, except for this one last glitch:

Expected:

Actual:

googlebot · 2018-01-11T16:15:55Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Closes angular#21094

mary-poppins · 2018-01-11T16:24:13Z

You can preview 0e4faca at https://pr21371-0e4faca.ngbuilds.io/.

mary-poppins · 2018-01-11T16:27:26Z

You can preview d7d825c at https://pr21371-d7d825c.ngbuilds.io/.

gkalpak

It should all be good now (let's see if Travis agrees).

petebacondarwin · 2018-01-11T18:41:36Z

✨

Closes #21094 PR Close #21371

Closes angular#21094 PR Close angular#21371

angular-automatic-lock-bot · 2019-09-13T04:50:57Z

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

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release comp: aio hotlist: devrel labels Jan 7, 2018

petebacondarwin requested review from StephenFluin, IgorMinar and gkalpak January 7, 2018 21:41

googlebot added the cla: yes label Jan 7, 2018

petebacondarwin added this to REVIEW in docs-infra Jan 7, 2018

petebacondarwin commented Jan 7, 2018

View reviewed changes

petebacondarwin force-pushed the aio-survey branch 2 times, most recently from 20476d6 to 0f3b280 Compare January 7, 2018 22:14

IgorMinar suggested changes Jan 8, 2018

View reviewed changes

petebacondarwin force-pushed the aio-survey branch from 0f3b280 to 9629552 Compare January 8, 2018 17:27

StephenFluin approved these changes Jan 8, 2018

View reviewed changes

petebacondarwin force-pushed the aio-survey branch from 9629552 to 835a0f0 Compare January 8, 2018 17:40

petebacondarwin force-pushed the aio-survey branch from 835a0f0 to 40a71b8 Compare January 8, 2018 18:20

petebacondarwin force-pushed the aio-survey branch from 40a71b8 to e70ddaa Compare January 8, 2018 18:56

gkalpak requested changes Jan 11, 2018

View reviewed changes

petebacondarwin force-pushed the aio-survey branch from dbd2e3c to 22c0489 Compare January 11, 2018 14:10

gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jan 11, 2018

gkalpak requested changes Jan 11, 2018

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jan 11, 2018

feat(aio): implement survey notification link

d7d825c

Closes angular#21094

gkalpak force-pushed the aio-survey branch from 0e4faca to d7d825c Compare January 11, 2018 16:18

gkalpak added cla: yes and removed cla: no labels Jan 11, 2018

gkalpak approved these changes Jan 11, 2018

View reviewed changes

gkalpak added action: merge The PR is ready for merge by the caretaker 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 Jan 11, 2018

alexeagle pushed a commit that referenced this pull request Jan 12, 2018

feat(aio): implement survey notification link (#21371)

9005a6f

Closes #21094 PR Close #21371

alexeagle closed this in 4f31492 Jan 12, 2018

petebacondarwin removed this from MERGE in docs-infra Jan 12, 2018

petebacondarwin deleted the aio-survey branch January 16, 2018 11:04

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(aio): implement survey notification link (angular#21371)

f705723

Closes angular#21094 PR Close angular#21371

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

feat(aio): add survey link #21371

feat(aio): add survey link #21371

Conversation

petebacondarwin commented Jan 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Jan 7, 2018

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMinar commented Jan 8, 2018 via email

mary-poppins commented Jan 8, 2018

mary-poppins commented Jan 8, 2018

mary-poppins commented Jan 8, 2018

petebacondarwin commented Jan 8, 2018

StephenFluin commented Jan 8, 2018

gkalpak left a comment

Choose a reason for hiding this comment

mary-poppins commented Jan 11, 2018

mary-poppins commented Jan 11, 2018

mary-poppins commented Jan 11, 2018

alexeagle commented Jan 11, 2018

gkalpak left a comment • edited

Choose a reason for hiding this comment

googlebot commented Jan 11, 2018

mary-poppins commented Jan 11, 2018

mary-poppins commented Jan 11, 2018

gkalpak left a comment

Choose a reason for hiding this comment

petebacondarwin commented Jan 11, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

gkalpak left a comment •

edited