iframe-proxy

gkalpak · 2019-07-02T17:10:52Z

I started to fix the "No results found" message being hidden, but ended up doing some clean up and other minor fixes (see individual commits for details).

WRT to the main fix ("No results found" being hidden):

BEFORE

AFTER

Both before and after the "No results found" is displayed correctly on the "Page Not Found" page:

UPDATE:

This PR also fixes a bug in ScrollService that caused test flakes (example). No idea why the flakes started appearing on this PR (which was just changing CSS styles 🤷‍♂).

mary-poppins · 2019-07-02T17:17:31Z

mary-poppins · 2019-07-03T15:13:54Z

You can preview 119dcd1 at https://pr31390-119dcd1.ngbuilds.io/.

sjtrimble · 2019-07-04T19:35:29Z

@gkalpak

A couple UX/UI recommendations:

Don't actually show the "I have nothing to show you" message if we don't have any suggestions OR even better (maybe handle it in another PR) show them a few items where they might want to spend some time on like the blog, the guide, etc.

Can we do something to fix the cut off face image at the top? :)

mary-poppins · 2019-07-05T14:13:42Z

You can preview a9b3ede at https://pr31390-a9b3ede.ngbuilds.io/.

gkalpak · 2019-07-05T14:27:02Z

Thx for taking a look, @sjtrimble. I fixed the image placement.

Regarding the "no results" message, I wouldn't be opposed to showing something (although I assume it should rarely come up for the page-not found error page). It is not related to this PR though (I am just fixing the color of the message to be different from the background and thus visible). Feel free to open an issue (or a PR 😉) about changing the message and we can discuss the details there.

sjtrimble · 2019-07-09T20:17:28Z

One more thing @gkalpak .. I'm still seeing the headers showing up a white and thereby transparent. Try seeing here: https://pr31390-119dcd1.ngbuilds.io/ngmodules

sjtrimble · 2019-07-09T20:20:05Z

I normally put a line break here so it's easier to read ... if you feel like it :)

I was keeping inline with the convention in this file, which seems to be "no line break, if it's targeting the same element". Happy to go either way.

I added the line breaks (since I agree it is more readable).

…ent with other `.scss` files

Previously, `.search-area` had a default style of `display: flex`, but it was overriden to `display: block` in media queries for `max-width: 480px` and `min-width: 600px`. As a result, it only had `display: flex` between 481px and 599px. Since no flex layout features are necessary inside `.search-area`, this commit changes the style to always be `display: block` (i.e. the default for `div` elements).

Previously, the `search-results` layout was switch from horizontal to vertical at 480px. Yes, since some search queries can yield more than 5 `.search-area`s, even 600px are too narrow to accomodate a horizontal layout. This commit changes the breakpoint at which the layout switches to vertical from 480px to 600px.

…ssage

The `ScrollService` sets up some global `window` listeners. Previously, these listeners were never unregistered. This was not a problem in the real app, because the `ScrollService` instance exists for the lifetime of a user session. In tests, however, where the `window` instance is among all tests, the listeners would survive the `ScrollService` tests. This, in addition to the fact that we used a mock `ViewportScroller` which did not return the expected type from `getScrollPosition()`, caused errors to be thrown in unrelated tests (i.e. whenever a scroll event was emitted on `window`). See [here][1] for an example failure. This commit fixes it by adding an `ngOnDestroy()` method that unregisters the listeners and ensuring it is called after each `ScrollService` test. [1]: https://circleci.com/gh/angular/angular/381649

The `.sidenav-content` element, [which is also][1] the `#file-not-found` element, [already has][2] the necessary padding. Especially the top padding is important to ensure the element is not obscured by the top-bar. [1]: https://github.com/angular/angular/blob/1d3e22766/aio/src/app/app.component.html#L51 [2]: https://github.com/angular/angular/blob/1d3e22766/aio/src/styles/1-layouts/_content-layout.scss#L10

gkalpak · 2019-07-09T20:41:18Z

I'm still seeing the headers showing up a white and thereby transparent.

Great catch! 😱 😱 😱
Fixed.

mary-poppins · 2019-07-09T20:47:02Z

You can preview 4be1fe9 at https://pr31390-4be1fe9.ngbuilds.io/.

mary-poppins · 2019-07-09T20:53:14Z

You can preview 9967ab9 at https://pr31390-9967ab9.ngbuilds.io/.

mary-poppins · 2019-07-09T20:57:14Z

You can preview 4fac6dd at https://pr31390-4fac6dd.ngbuilds.io/.

PR Close #31390

Previously, `.search-area` had a default style of `display: flex`, but it was overriden to `display: block` in media queries for `max-width: 480px` and `min-width: 600px`. As a result, it only had `display: flex` between 481px and 599px. Since no flex layout features are necessary inside `.search-area`, this commit changes the style to always be `display: block` (i.e. the default for `div` elements). PR Close #31390

…31390) Previously, the `search-results` layout was switch from horizontal to vertical at 480px. Yes, since some search queries can yield more than 5 `.search-area`s, even 600px are too narrow to accomodate a horizontal layout. This commit changes the breakpoint at which the layout switches to vertical from 480px to 600px. PR Close #31390

…ssage (#31390) PR Close #31390

…ts (#31390) The `ScrollService` sets up some global `window` listeners. Previously, these listeners were never unregistered. This was not a problem in the real app, because the `ScrollService` instance exists for the lifetime of a user session. In tests, however, where the `window` instance is among all tests, the listeners would survive the `ScrollService` tests. This, in addition to the fact that we used a mock `ViewportScroller` which did not return the expected type from `getScrollPosition()`, caused errors to be thrown in unrelated tests (i.e. whenever a scroll event was emitted on `window`). See [here][1] for an example failure. This commit fixes it by adding an `ngOnDestroy()` method that unregisters the listeners and ensuring it is called after each `ScrollService` test. [1]: https://circleci.com/gh/angular/angular/381649 PR Close #31390

The `.sidenav-content` element, [which is also][1] the `#file-not-found` element, [already has][2] the necessary padding. Especially the top padding is important to ensure the element is not obscured by the top-bar. [1]: https://github.com/angular/angular/blob/1d3e22766/aio/src/app/app.component.html#L51 [2]: https://github.com/angular/angular/blob/1d3e22766/aio/src/styles/1-layouts/_content-layout.scss#L10 PR Close #31390

…ent with other `.scss` files (#31390) PR Close #31390

PR Close #31390

Previously, `.search-area` had a default style of `display: flex`, but it was overriden to `display: block` in media queries for `max-width: 480px` and `min-width: 600px`. As a result, it only had `display: flex` between 481px and 599px. Since no flex layout features are necessary inside `.search-area`, this commit changes the style to always be `display: block` (i.e. the default for `div` elements). PR Close #31390

…31390) Previously, the `search-results` layout was switch from horizontal to vertical at 480px. Yes, since some search queries can yield more than 5 `.search-area`s, even 600px are too narrow to accomodate a horizontal layout. This commit changes the breakpoint at which the layout switches to vertical from 480px to 600px. PR Close #31390

…ssage (#31390) PR Close #31390

…ts (#31390) The `ScrollService` sets up some global `window` listeners. Previously, these listeners were never unregistered. This was not a problem in the real app, because the `ScrollService` instance exists for the lifetime of a user session. In tests, however, where the `window` instance is among all tests, the listeners would survive the `ScrollService` tests. This, in addition to the fact that we used a mock `ViewportScroller` which did not return the expected type from `getScrollPosition()`, caused errors to be thrown in unrelated tests (i.e. whenever a scroll event was emitted on `window`). See [here][1] for an example failure. This commit fixes it by adding an `ngOnDestroy()` method that unregisters the listeners and ensuring it is called after each `ScrollService` test. [1]: https://circleci.com/gh/angular/angular/381649 PR Close #31390

The `.sidenav-content` element, [which is also][1] the `#file-not-found` element, [already has][2] the necessary padding. Especially the top padding is important to ensure the element is not obscured by the top-bar. [1]: https://github.com/angular/angular/blob/1d3e22766/aio/src/app/app.component.html#L51 [2]: https://github.com/angular/angular/blob/1d3e22766/aio/src/styles/1-layouts/_content-layout.scss#L10 PR Close #31390

angular-automatic-lock-bot · 2019-09-15T07:21:51Z

gkalpak requested a review from a team July 2, 2019 17:10

googlebot added the cla: yes label Jul 2, 2019

gkalpak requested a review from sjtrimble July 2, 2019 17:11

gkalpak added comp: docs-infra action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release type: bug/fix labels Jul 2, 2019

ngbot Bot added this to the needsTriage milestone Jul 2, 2019

gkalpak modified the milestones: needsTriage, docs-infra-aio-app Jul 2, 2019

gkalpak force-pushed the fix-aio-no-search-results branch from 9489a5b to 119dcd1 Compare July 3, 2019 15:06

petebacondarwin approved these changes Jul 3, 2019

View reviewed changes

gkalpak force-pushed the fix-aio-no-search-results branch from 119dcd1 to a9b3ede Compare July 5, 2019 14:07

sjtrimble 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 Jul 9, 2019

sjtrimble suggested changes Jul 9, 2019

View reviewed changes

gkalpak added 7 commits July 9, 2019 23:26

style(docs-infra): make indentation in _search-results.scss consist…

fefe30b

…ent with other `.scss` files

refactor(docs-infra): clean up aio-search-results styles

f572e56

fix(docs-infra): show (and style correctly) the 'No results found' me…

d8a269b

…ssage

gkalpak force-pushed the fix-aio-no-search-results branch from a9b3ede to 4be1fe9 Compare July 9, 2019 20:40

gkalpak force-pushed the fix-aio-no-search-results branch from 4be1fe9 to 9967ab9 Compare July 9, 2019 20:45

fixup! refactor(docs-infra): clean up aio-search-results styles

4fac6dd

gkalpak force-pushed the fix-aio-no-search-results branch from 9967ab9 to 4fac6dd Compare July 9, 2019 20:46

sjtrimble self-requested a review July 10, 2019 19:16

sjtrimble approved these changes Jul 10, 2019

View reviewed changes

gkalpak removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 10, 2019

matsko closed this in 2d4f507 Jul 11, 2019

matsko pushed a commit that referenced this pull request Jul 11, 2019

refactor(docs-infra): clean up aio-search-results styles (#31390)

75b6b0e

PR Close #31390

matsko pushed a commit that referenced this pull request Jul 11, 2019

fix(docs-infra): show (and style correctly) the 'No results found' me…

9b29ca9

…ssage (#31390) PR Close #31390

matsko pushed a commit that referenced this pull request Jul 11, 2019

style(docs-infra): make indentation in _search-results.scss consist…

705670b

…ent with other `.scss` files (#31390) PR Close #31390

matsko pushed a commit that referenced this pull request Jul 11, 2019

refactor(docs-infra): clean up aio-search-results styles (#31390)

e5d545a

PR Close #31390

matsko pushed a commit that referenced this pull request Jul 11, 2019

fix(docs-infra): show (and style correctly) the 'No results found' me…

fa2773d

…ssage (#31390) PR Close #31390

gkalpak deleted the fix-aio-no-search-results branch July 11, 2019 16:51

sjtrimble mentioned this pull request Jul 12, 2019

Improve bad URL experience #31532

Closed

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

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

gkalpak commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mary-poppins commented Jul 2, 2019

Uh oh!

mary-poppins commented Jul 3, 2019

Uh oh!

sjtrimble commented Jul 4, 2019

Uh oh!

mary-poppins commented Jul 5, 2019

Uh oh!

gkalpak commented Jul 5, 2019

Uh oh!

sjtrimble commented Jul 9, 2019

Uh oh!

sjtrimble Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

gkalpak Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

gkalpak Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Jul 9, 2019

Uh oh!

mary-poppins commented Jul 9, 2019

Uh oh!

mary-poppins commented Jul 9, 2019

Uh oh!

mary-poppins commented Jul 9, 2019

Uh oh!

angular-automatic-lock-bot Bot commented Sep 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gkalpak commented Jul 2, 2019 •

edited

Loading