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.

Sign up for GitHub

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

Jump to bottom

aio: add search to 404 page #19682

Closed
wants to merge 7 commits into from
Closed

aio: add search to 404 page #19682

wants to merge 7 commits into from

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Oct 12, 2017 •
edited

... and a few other tidy ups; so it is best to review commit by commit.

Closes #16656

@petebacondarwin petebacondarwin added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 12, 2017
@petebacondarwin petebacondarwin added this to REVIEW in docs-infra Oct 12, 2017
@mary-poppins
Copy link

You can preview 95e2bf6 at https://pr19682-95e2bf6.ngbuilds.io/.

@petebacondarwin
Copy link
Member Author

An example: https://pr19682-95e2bf6.ngbuilds.io/testing/router

gkalpak
gkalpak approved these changes Oct 12, 2017
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Awesome! (A couple of minor comments, but LGTM anyway 👍)

aio/tsconfig.json
@@ -8,6 +8,7 @@
"moduleResolution": "node",
"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"noUnusedLocals": true,
Copy link
Member

Choose a reason for hiding this comment

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

❤️ 💯 ❤️

aio/src/app/search/search.service.spec.ts Outdated
service.searchResults.subscribe(results => searchResults = results);
service.search('some query');
expect(searchResults).toEqual(mockSearchResults);
const $searchResults = service.search('some query');
Copy link
Member

Choose a reason for hiding this comment

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

I thought the $ was supposed to go at the end 😛

aio/src/app/search/search-results/search-results.component.spec.ts Outdated

searchResults.next({ query: '', results: results});
setSearchResults('', getTestResults(3));
component.ngOnChanges({});
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary (here and below), since setSearchResults() calls it anyway?

aio/content/file-not-found.md Outdated
@@ -5,5 +5,7 @@
<div class="nf-response l-flex-wrap">
<h1 class="no-toc">Page Not Found</h1>
<p>We're sorry. The page you are looking for cannot be found.</p>
<p>Perhaps you were looking for something else...</p>
Copy link
Member

Choose a reason for hiding this comment

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

I would remove that from here and add a heading right above the search results explaining what they are.
Right now it is not exactly obvious what all these words under the main 404 page are (they are search results 😁).

@gkalpak 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: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 12, 2017
@petebacondarwin petebacondarwin 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 Oct 12, 2017
@petebacondarwin petebacondarwin moved this from REVIEW to MERGE in docs-infra Oct 12, 2017
@chuckjaz
Copy link
Contributor

@petebacondarwin The AIO test failure looks real. It appears aio is now over one of the size limits.

@chuckjaz
Copy link
Contributor

@petebacondarwin Can you also set the PR target? I assume it is master and patch.

@chuckjaz chuckjaz added this to the Merge-queue milestone Oct 12, 2017
@petebacondarwin petebacondarwin added the target: patch This PR is targeted for the next patch release label Oct 12, 2017
@petebacondarwin
Copy link
Member Author

Sorry @chuckjaz - I have added the target and bumped the payload limit a bit more.

@mary-poppins
Copy link

You can preview ee92b40 at https://pr19682-ee92b40.ngbuilds.io/.

@tbosch
Copy link
Contributor

tbosch commented Oct 13, 2017

@petebacondarwin As this doesn't apply cleanly to 4.4.x, could you create another PR that adds this change to 4.4.x? My guess would be to skip the commit for bumping rxjs, but having another PR also proofs that this will really work on 4.4.x.

@tbosch
Copy link
Contributor

tbosch commented Oct 13, 2017

Changed the target to be masteronly for this PR...

@tbosch tbosch added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Oct 13, 2017
@tbosch tbosch closed this in 09b4244 Oct 13, 2017
tbosch pushed a commit that referenced this pull request Oct 13, 2017
@petebacondarwin @tbosch
build(aio): remove unused imports and local variables (#19682)
717c680 
PR Close #19682
tbosch pushed a commit that referenced this pull request Oct 13, 2017
@petebacondarwin @tbosch
test(aio): tidy up e2e tests that used invalid URLs (#19682)
6121083 
PR Close #19682
tbosch pushed a commit that referenced this pull request Oct 13, 2017
@petebacondarwin @tbosch
feat(aio): allow SearchService to have multiple clients (#19682)
c3f07b3 
PR Close #19682
tbosch pushed a commit that referenced this pull request Oct 13, 2017
@petebacondarwin @tbosch
refactor(aio): move SearchResultsComponent into shared module (#19682)
88c46fe 
This will allow it to be used by an embedded component.

PR Close #19682
tbosch pushed a commit that referenced this pull request Oct 13, 2017
@petebacondarwin @tbosch
feat(aio): add search to 404 page (#19682)
91fcfcb 
The 404 page will now run a search based on the given URL to offer
suggestions for the page that the user really wanted.

PR Close #19682
tbosch pushed a commit that referenced this pull request Oct 13, 2017
@petebacondarwin @tbosch
ci(aio): raise payload limit to accommodate the new search feature (#…
97969a8 
…19682)

PR Close #19682
@petebacondarwin petebacondarwin mentioned this pull request Oct 13, 2017
Closed
@petebacondarwin petebacondarwin removed this from MERGE in docs-infra Oct 13, 2017
@petebacondarwin petebacondarwin deleted the aio-search-on-404 branch November 17, 2017 13:04
@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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gkalpak gkalpak gkalpak approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
Merge-queue
Development

Successfully merging this pull request may close these issues.

feat(aio): add search to 404 page
6 participants
@petebacondarwin @mary-poppins @chuckjaz @tbosch @gkalpak @googlebot