-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[App Search] Empty Crawler Single Domain view #107694
[App Search] Empty Crawler Single Domain view #107694
Conversation
pageHeader={{ pageTitle: displayDomainUrl }} | ||
isLoading={dataLoading} | ||
> | ||
<EuiCode>{JSON.stringify(domain, null, 2)}</EuiCode> |
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.
I like the incremental approach with this PR. ++
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 Rich, we've done it on a few other views, I've found it helpful to show that something's happening on the BE even if we don't have anything fancy to show
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.
+++
c0fa550
to
09b35cf
Compare
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.
Overall looks good. Posted a few questions and suggestions but nothing critical.
...rise_search/public/applications/app_search/components/crawler/crawler_single_domain.test.tsx
Outdated
Show resolved
Hide resolved
const { fetchDomainData } = useActions(CrawlerSingleDomainLogic); | ||
|
||
const displayDomainUrl = domain | ||
? domain.url |
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.
Aren't we displaying the domain name only in the standalone UI? Is that something you wanna follow up with or do in this PR?
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.
domain.url
is domain.name
in the API which I believe is only the domain + protocol, and does not include any entry points
pageHeader={{ pageTitle: displayDomainUrl }} | ||
isLoading={dataLoading} | ||
> | ||
<EuiCode>{JSON.stringify(domain, null, 2)}</EuiCode> |
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.
+++
...search/public/applications/app_search/components/crawler/crawler_single_domain_logic.test.ts
Outdated
Show resolved
Hide resolved
|
||
try { | ||
const response = await http.get( | ||
`/api/app_search/engines/${engineName}/crawler/domains/${domainId}` |
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.
In this context, this should be OK but I would suspect that with analytics for instance we would want/need to escape the dynamic parts of the URL path.
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.
Can you elaborate on this? I'm not sure I understand why we would want/need to escape the dynamic parts in the context of analytics.
@@ -66,6 +66,21 @@ export function registerCrawlerRoutes({ | |||
}) | |||
); | |||
|
|||
router.get( | |||
{ | |||
path: '/api/app_search/engines/{name}/crawler/domains/{id}', |
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.
The name and id params here read a bit weird to me because they could appear to be related but one if the engine name and the other is the domain ID. I see this is consistent with the other routes but perhaps something to look into and clear up later.
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.
Yeah this follows existing but not great patterns, in the future I would like us to be more specific with these names.
Co-authored-by: Orhan Toy <[email protected]>
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
* New route to retreive data for a single domain * New CrawlerSingleDomainLogic logic * New CrawlerSingleDomain view component * Add CrawlerSingleDomain to CrawlerRouter * Use different default text for page title while loading * Apply suggestions from code review Co-authored-by: Orhan Toy <[email protected]> Co-authored-by: Orhan Toy <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* New route to retreive data for a single domain * New CrawlerSingleDomainLogic logic * New CrawlerSingleDomain view component * Add CrawlerSingleDomain to CrawlerRouter * Use different default text for page title while loading * Apply suggestions from code review Co-authored-by: Orhan Toy <[email protected]> Co-authored-by: Orhan Toy <[email protected]> Co-authored-by: Byron Hulcher <[email protected]> Co-authored-by: Orhan Toy <[email protected]>
* New route to retreive data for a single domain * New CrawlerSingleDomainLogic logic * New CrawlerSingleDomain view component * Add CrawlerSingleDomain to CrawlerRouter * Use different default text for page title while loading * Apply suggestions from code review Co-authored-by: Orhan Toy <[email protected]> Co-authored-by: Orhan Toy <[email protected]>
Summary
Sets up a view and related routes for the Crawler Single Domain view. We retrieve the data for the domain on load and display it in an
EuiCode
.Screenshots
Checklist
Delete any items that are not applicable to this PR.