-
Notifications
You must be signed in to change notification settings - Fork 20
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.
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
Check and redirect to search query from 404 page, if results exist #1291
base: main
Are you sure you want to change the base?
Check and redirect to search query from 404 page, if results exist #1291
Conversation
28952b0
to
f4aedc7
Compare
I will try to review this by the end of the day. You will probably need to update or add playwright tests to cover both the badID provided and not provided cases. And will want to add some unit tests too since this is now adding logic to the feature not found page. Could you also chime in on #1263 with your specs and settings? |
@jcscottiii I will add playwright test for To implement search in webstatus and chrome-dashboard, can I use RediSearch or is ElasticSearch better. |
The work for that proposal will happen primarily in https://github.com/GoogleChrome/chromium-dashboard then later be ported to here. You should create an issue in https://github.com/GoogleChrome/chromium-dashboard As a heads up, we try to use managed services in GCP. This reduces the maintenance burden. So if you're looking into RediSearch, check if Memorystore for Redis supports it (docs). Plus, the chromium-dashboard already uses redis so it simplifies the changes needed to the infrastructure. I don't think there's a managed version of ElasticSearch in GCP. But feel free to see if there are other GCP managed services that could help. These are all just some of my thoughts. But you should create the issue in that repo to start the discussion before diving too deep. cc: @jrobbins for context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suprith-hub For this change, I see that you are calling getFeatures to check if there are any features with similar names.
All that said, I would prefer to show suggestions like mentioned in #1214 but I'm not sure how much time you have to commit to this. But I appreciate any and all contributions. If you don't have much time, just show the second button to search for similar features.
If we are going to call getFeatures, we should present some of the options. Otherwise, we could just add the second button alongside the existing go home button.
Also, you should use Lit's Task for making asynchronous network calls like that instead of using the apiclient directly. More about it here: https://lit.dev/docs/data/task/
Also, I would move the API client call itself to the notfound error page.
Other notes:
We should move the creation of the 404 feature page URL string to https://github.com/GoogleChrome/webstatus.dev/blob/main/frontend/src/static/js/utils/urls.ts
Also inside that URL.ts helper, you should do the retrieval of the feature ID instead of pulling from the window
object directly.
@jcscottiii about showing similar results, |
It is not meant to replace the overview page. But only include suggestions. It does not need to show all of the possible results like the overview page. Instead, it could:
The list and the first button would only appear if it found features. Hopefully that makes sense. Let me know if you have any questions. |
@jcscottiii I will make the changes by tomorrow, and |
|
@suprith-hub Thank you for the updates with the three screenshots/cases. We should ignore the second case for now. If there are no suggestions, there's no need to show the Search for similar features button. |
aeab3ab
to
945f62c
Compare
@jcscottiii I have written tests and made latest push, I think the feature is almost complete now. Can you review it... |
@suprith-hub I have not forgotten about this. I will get around to reviewing this tomorrow. |
Yeah, no worries at all! I figured you might’ve scheduled it — thanks for letting me know! 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Take a look at these comments and let me know if you have any questions.
frontend/src/static/js/components/webstatus-notfound-error-page.ts
Outdated
Show resolved
Hide resolved
frontend/src/static/js/components/webstatus-notfound-error-page.ts
Outdated
Show resolved
Hide resolved
frontend/src/static/js/components/webstatus-notfound-error-page.ts
Outdated
Show resolved
Hide resolved
frontend/src/static/js/components/webstatus-notfound-error-page.ts
Outdated
Show resolved
Hide resolved
frontend/src/static/js/components/webstatus-notfound-error-page.ts
Outdated
Show resolved
Hide resolved
frontend/src/static/js/components/webstatus-notfound-error-page.ts
Outdated
Show resolved
Hide resolved
protected render(): TemplateResult { | ||
const featureId = getSearchQuery(this.location); | ||
const taskState = this._similarResults?.value; | ||
const hasSimilar = Array.isArray(taskState) && taskState.length > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcscottiii Can I declares variables to check statuses, here... is it a good way?
@jcscottiii I have made the changes.
|
b5297cf
to
bacb36b
Compare
@jcscottiii I have rebased the branch and made latest changes, I think the PR can be reviewed now. |
@jcscottiii Could you please review this?
I have two main points to discuss:
1. Handling Bad IDs:
Currently, I'm making an API call to fetch a single feature with the bad ID.
Based on the response, I either show "Go to home" or "Search similar names" options.
Other implementations I've seen seem to directly redirect to the homepage instead of a dedicated 404 page.
Do you think this approach is better, or should we handle it differently?
2. Placement of
handleNotFound
function:I've placed
handleNotFound
in [mention file name].Is this the right place, or would it be better suited elsewhere?
This fix addressing the issue: #1214