feat(docs-infra): add useful links if landed on 404 page by ajitsinghkaler · Pull Request #34978 · angular/angular · GitHub
Skip to content

feat(docs-infra): add useful links if landed on 404 page - #34978

Closed
ajitsinghkaler wants to merge 3 commits into
angular:masterfrom
ajitsinghkaler:404-page
Closed

feat(docs-infra): add useful links if landed on 404 page#34978
ajitsinghkaler wants to merge 3 commits into
angular:masterfrom
ajitsinghkaler:404-page

Conversation

@ajitsinghkaler

@ajitsinghkaler ajitsinghkaler commented Jan 26, 2020

Copy link
Copy Markdown
Contributor

Added additional links useful links which can help user find the things he is looking for on the 404 page
Fixes #31532

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

On no link found no additional shown which can be helpful

Issue Number: 31532

What is the new behavior?

Added additional links which can be helpful to users to find what they are looking for

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

gkalpak
gkalpak suggested changes Jan 27, 2020
View reviewed changes

@gkalpak gkalpak left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thx for taking this on, @ajitsinghkaler! Per the discussion in #31532, the generic links would appear if there can be no relevant search results. (I would be fine putting them below the search result too.)

So, they need to be somehow incorparated into the aio-file-not-found-search component.

@gkalpak gkalpak added comp: docs-infra state: WIP target: patch This PR is targeted for the next patch release feature Label used to distinguish feature request from other issues labels Jan 27, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Jan 27, 2020
@ajitsinghkaler
ajitsinghkaler requested a review from a team January 27, 2020 18:27
@ajitsinghkaler
ajitsinghkaler force-pushed the 404-page branch 2 times, most recently from 19aee04 to e4c6619 Compare January 30, 2020 16:23
@ajitsinghkaler

ajitsinghkaler commented Feb 1, 2020

Copy link
Copy Markdown
Contributor Author

@ajitsinghkaler

ajitsinghkaler commented Feb 4, 2020

Copy link
Copy Markdown
Contributor Author

@gkalpak can you please look into this too

@mary-poppins

mary-poppins commented Feb 4, 2020

Copy link
Copy Markdown

You can preview 99c2faf at https://pr34978-99c2faf.ngbuilds.io/.
You can preview 19aee04 at https://pr34978-19aee04.ngbuilds.io/.
You can preview e4c6619 at https://pr34978-e4c6619.ngbuilds.io/.

gkalpak
gkalpak suggested changes Feb 5, 2020
View reviewed changes

@gkalpak gkalpak left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Regarding why the e2e tests are failing:

The SearchResultsComponent will initially have an empty result set (since it hasn't received the search results yet) and will therefore show the #notFound template. At that point, however, the notFoundMessage property is set to "Searching...".

Then, once the results are received, the notFoundMessage is changed to "No results found" and either the #searchResults or the #notFound templates are shown (the latter now showing "No results found" instead of "Searching...").

So, while we pretend there are two states (no results or results), there are actually three: searching, no results, results
But we are piggybacking on the "no results" state to handle the "searching" case too.

Now, why this causes the e2e to fail?

In the e2e tests, we send a search query and run assertions against the search results. The way we retrieve the search results is by waiting for elements that match .search-results li.

Previously, such elements would only be present when there were some actual search results. With your changes, these elements are also present whenever the #notFound template is shown, which (as explained above) is also showing while the search is in progress.

As a result, the e2e tests will look at the generic links and think they are the search results returned by the search query and thus try to run assertions on them. These assertions will fail, of course, because these "results" do not contain the expected terms.

How to fix this?

I think the proper fix is to have three templates:

  • One for when there are results (i.e. the #searchResults template we already have).
  • One for when there are no results showing the generic links (i.e. the #notFound template, except with the notFoundMessage hard-coded to "No results found.").
  • One for when the search is in progress without the generic links (and with a hard-coded "Searching..." message).

Then we would also need a property on the component to track whether a search is in progress or not.

LMK if that makes sense and whether you need any help with the implementation.

Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/styles/2-modules/_search-results.scss Outdated
Comment thread aio/src/styles/2-modules/_search-results.scss Outdated
Comment thread aio/src/styles/2-modules/_search-results.scss Outdated
@mary-poppins

mary-poppins commented Feb 5, 2020

Copy link
Copy Markdown

You can preview 4da55d9 at https://pr34978-4da55d9.ngbuilds.io/.

@ajitsinghkaler

ajitsinghkaler commented Feb 9, 2020

Copy link
Copy Markdown
Contributor Author

@gkalpak I've made the changes pleas have a look

@mary-poppins

mary-poppins commented Feb 9, 2020

Copy link
Copy Markdown

You can preview 6076979 at https://pr34978-6076979.ngbuilds.io/.

@mary-poppins

mary-poppins commented Feb 9, 2020

Copy link
Copy Markdown

You can preview fed6345 at https://pr34978-fed6345.ngbuilds.io/.

@ajitsinghkaler

ajitsinghkaler commented Feb 9, 2020

Copy link
Copy Markdown
Contributor Author

@gkalpak why these hooks are failing can you please explain this.

gkalpak
gkalpak suggested changes Feb 14, 2020
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thx for implementing this, @ajitsinghkaler! It is much cleaner now.
Can you also update the unit tests to at least verify when the extra links are shown?

Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.html Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.ts Outdated
Comment thread aio/src/app/shared/search-results/search-results.component.ts Outdated
@gkalpak gkalpak added the action: merge The PR is ready for merge by the caretaker label Feb 26, 2020
@ngbot

ngbot Bot commented Feb 26, 2020

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: legacy-unit-tests-saucelabs" is failing
    failure status "ci/circleci: test_aio" is failing
    failure status "ci/circleci: test_aio_local" is failing
    failure status "ci/circleci: test_aio_local_viewengine" is failing
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@gkalpak

gkalpak commented Feb 26, 2020

Copy link
Copy Markdown
Member

Hm...this PR seems to increase the main bundle by 1KB. Investigating whether that's expected...

@mary-poppins

mary-poppins commented Feb 27, 2020

Copy link
Copy Markdown

You can preview 57563b0 at https://pr34978-57563b0.ngbuilds.io/.

gkalpak and others added 3 commits February 27, 2020 13:13
@gkalpak
ci(docs-infra): increase AIO ViewEngine payload size limit
68c82c5 
In angular#35702, the payload size limit for Ivy builds was bumped to account
for small incremental increases in recent PRs. The ViewEngine size has
also increased similarly (~500B), but it was not updated in angular#35702,
because its total increase was just below the 500B error threshold (by
6B).

This commit bumps the ViewEngine size limit too.

Note: Any investigation for the Ivy size increase (as a follow-up
to angular#35702) will most likely also apply to ViewEngine, since the size was
increased by the same amount.
@ajitsinghkaler @gkalpak
feat(docs-infra): add useful links if landed on 404 page and no searc…
6a0fb68 
…h results found

Added additional links which can help user find the things they are
looking for when there are no search results (when searching or on a 404
page).

Note:
This commit increases the main bundle's payload size due to the extra
content of the `aio-search-results` component.

Fixes angular#31532
@gkalpak
fixup! feat(docs-infra): add useful links if landed on 404 page and n…
383a277 
…o search results found
@googlebot

This comment has been minimized.

Sign in to view
@pullapprove
pullapprove Bot requested a review from kara February 27, 2020 11:26
@gkalpak

This comment has been minimized.

Sign in to view
@googlebot

This comment has been minimized.

Sign in to view
@gkalpak

gkalpak commented Feb 27, 2020

Copy link
Copy Markdown
Member

OK, it seems that the increase is expected (and is caused by the changes and extra content in the aio-search-results component). For Ivy, it is ~1KB (uncompressed).

I rebased on master, updated payload size limits for current master (to make it easy to see the increase by this PR) and bumped the payload size limits due to this PR's changes.

@mary-poppins

mary-poppins commented Feb 27, 2020

Copy link
Copy Markdown

You can preview 383a277 at https://pr34978-383a277.ngbuilds.io/.

atscott pushed a commit that referenced this pull request Feb 27, 2020
@gkalpak @atscott
ci(docs-infra): increase AIO ViewEngine payload size limit (#34978)
e4f05d1 
In #35702, the payload size limit for Ivy builds was bumped to account
for small incremental increases in recent PRs. The ViewEngine size has
also increased similarly (~500B), but it was not updated in #35702,
because its total increase was just below the 500B error threshold (by
6B).

This commit bumps the ViewEngine size limit too.

Note: Any investigation for the Ivy size increase (as a follow-up
to #35702) will most likely also apply to ViewEngine, since the size was
increased by the same amount.

PR Close #34978
atscott pushed a commit that referenced this pull request Feb 27, 2020
@ajitsinghkaler @atscott
feat(docs-infra): add useful links if landed on 404 page and no searc…
bf6fbf5 
…h results found (#34978)

Added additional links which can help user find the things they are
looking for when there are no search results (when searching or on a 404
page).

Note:
This commit increases the main bundle's payload size due to the extra
content of the `aio-search-results` component.

Fixes #31532

PR Close #34978
@atscott atscott closed this in aec463c Feb 27, 2020
atscott pushed a commit that referenced this pull request Feb 27, 2020
@ajitsinghkaler @atscott
feat(docs-infra): add useful links if landed on 404 page and no searc…
01ab168 
…h results found (#34978)

Added additional links which can help user find the things they are
looking for when there are no search results (when searching or on a 404
page).

Note:
This commit increases the main bundle's payload size due to the extra
content of the `aio-search-results` component.

Fixes #31532

PR Close #34978
ajitsinghkaler added a commit to ajitsinghkaler/angular that referenced this pull request Mar 5, 2020
@ajitsinghkaler
fix(docs-infra): in 404 page some text is not visible
accd277 
In pr angular#34978 colors were not properly set, if we type wrong url in the browser and we are directed to the 404 page there some text is set to white color which as not visible set it to dark gray for visibility
@ajitsinghkaler ajitsinghkaler mentioned this pull request Mar 5, 2020
Closed
3 of 14 tasks
matsko pushed a commit that referenced this pull request Mar 5, 2020
@ajitsinghkaler @matsko
fix(docs-infra): in 404 page some text is not visible (#35866)
876aa5a 
In pr #34978 colors were not properly set, if we type wrong url in the browser and we are directed to the 404 page there some text is set to white color which as not visible set it to dark gray for visibility

PR Close #35866
matsko pushed a commit that referenced this pull request Mar 5, 2020
@ajitsinghkaler @matsko
fix(docs-infra): in 404 page some text is not visible (#35866)
78f968b 
In pr #34978 colors were not properly set, if we type wrong url in the browser and we are directed to the 404 page there some text is set to white color which as not visible set it to dark gray for visibility

PR Close #35866
@ajitsinghkaler
ajitsinghkaler deleted the 404-page branch March 6, 2020 04:20
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Apr 6, 2020

Copy link
Copy Markdown

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kara kara Awaiting requested review from kara
1 more reviewer
@gkalpak gkalpak gkalpak approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes feature Label used to distinguish feature request from other issues target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

docs-infra-aio-app

Development

Successfully merging this pull request may close these issues.

Improve bad URL experience

4 participants

@ajitsinghkaler @mary-poppins @gkalpak @googlebot