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.

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

Obsolete PR #196338

Closed
wants to merge 19 commits into from
Closed

Obsolete PR #196338

wants to merge 19 commits into from

Conversation

JoseLuisGJ
Copy link
Contributor

@JoseLuisGJ JoseLuisGJ commented Oct 15, 2024

This PR was moved to this one: #199284

@JoseLuisGJ JoseLuisGJ self-assigned this Oct 17, 2024
@JoseLuisGJ JoseLuisGJ requested a review from sphilipse October 18, 2024 13:55
@JoseLuisGJ JoseLuisGJ marked this pull request as ready for review October 21, 2024 09:31
@JoseLuisGJ JoseLuisGJ requested a review from a team as a code owner October 21, 2024 09:31
@JoseLuisGJ JoseLuisGJ marked this pull request as draft October 21, 2024 09:33
@JoseLuisGJ JoseLuisGJ marked this pull request as ready for review October 21, 2024 09:34
@JoseLuisGJ JoseLuisGJ marked this pull request as draft October 21, 2024 09:34
Copy link
Contributor Author

@JoseLuisGJ JoseLuisGJ left a comment

Choose a reason for hiding this comment

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

I commented some concerns there @sphilipse

Copy link
Member

@sphilipse sphilipse left a comment

Choose a reason for hiding this comment

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

Thanks Jose! I added a bunch of comments, some of these are just reminders so that we don't forget to fix these things before merging.

<EuiButtonEmpty
data-test-subj="serverlessSearchElasticManagedWebCrawlerEmptyBackButton"
iconType="arrowLeft"
onClick={() => navigateToUrl(`./`)}
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be navigating back this way, but by explicitly going to a complete URL.

<EuiFlexItem grow={false}>
<EuiStepsHorizontal
css={css`
pointer-events: none;
Copy link
Member

Choose a reason for hiding this comment

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

Don't disable a pointer shape using CSS: these items are still interactive and clickable. People using screenreaders or tabbing through will still see these steps as interactive. If you want the visual element of steps but no interactivity, you should find a different solution than using interactive elements.

Copy link
Contributor Author

@JoseLuisGJ JoseLuisGJ Nov 6, 2024

Choose a reason for hiding this comment

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

I've been discussing this issue with the EUI team. And this a11y issue can be avoid using the inert HTML attribute to prevent click and focus events for all this content. In addition using the role="presentation" attribute we don't expose it to the accessibility tree.

With all that <EuiStepsHorizontal> component is not interactive with either the keyboard or the mouse:

CleanShot 2024-11-06 at 09 26 11

<EuiStepsHorizontal
   css={css`
      pointer-events: none;
   `}
    steps={horizontalSteps}
    size="s"
    role="presentation"
    // @ts-ignore
    inert=""
/>

@@ -0,0 +1,64 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

These next two components are extremely similar. We can probably combine them into one component, that just takes some inputs

export const FILE_UPLOAD_PATH = '/app/ml/filedatavisualizer';

export const CRAWLER = {
Copy link
Member

Choose a reason for hiding this comment

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

Are you expecting to add more things to these objects? Otherwise I would just export two constants called CRAWLER_GITHUB_LINK and CONNECTORS_GITHUB_LINK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I'm getting these values to be centrilised:

export const CRAWLER = {
  github_repo: 'https://github.com/elastic/crawler',
  docker_doc:
    'https://github.com/elastic/crawler?tab=readme-ov-file#running-open-crawler-with-docker',
};

export const CONNECTORS = {
  github_repo: 'https://github.com/elastic/connectors',
  self_managed_docs:
    'https://www.elastic.co/docs/current/serverless/elasticsearch/ingest-data-through-integrations-connector-client',
  docker_doc: 'https://github.com/elastic/connectors/blob/main/scripts/stack/README.md',
};

x-pack/plugins/serverless_search/public/navigation_tree.ts Outdated Show resolved Hide resolved
x-pack/plugins/serverless_search/public/plugin.ts Outdated Show resolved Hide resolved
export const EDIT_CONNECTOR_PATH = `${BASE_CONNECTORS_PATH}/:id`;
export const ELASTIC_MANAGED_CONNECTOR_PATH = '/elastic_managed';
Copy link
Member

Choose a reason for hiding this comment

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

Better to construct these the way the edit connector path above is constructed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I modify these constants in this:

export const ELASTIC_MANAGED_CONNECTOR_PATH = `${BASE_CONNECTORS_PATH}/elastic_managed`;
export const ELASTIC_MANAGED_WEB_CRAWLERS_PATH = `${BASE_WEB_CRAWLERS_PATH}/elastic_managed`;

Consuming them from the connectors_router.tsx like:

<Route exact path={ELASTIC_MANAGED_CONNECTOR_PATH}>
  <ConnectorsElasticManaged />
</Route>

Generates a non working route URL:

http://localhost:5601/app/connectors/connectors/elastic_managed

With a duplicated connectors/connectors...

@@ -0,0 +1,367 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Both this component and the elastic managed web crawlers component have a lot of stuff in common. Instead of duplicating tons of code, we should find ways to re-use some.


import { CONNECTORS } from '../constants';

export const ConnectorsElasticManaged = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a full connectors managed page here? We don't have any elastic managed connectors yet, and you're create connector creates a non-managed connector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This page aims for rendering the Coming soon page content for connectors as we do also with Web Crawlers. Consuming the and rendering as children the

<EuiPageTemplate.Section restrictWidth color="subdued">
        <ElasticManagedConnectorComingSoon />
 </EuiPageTemplate.Section>

We want to track how many users open this page so I guess this is the way to do it and get a dedicated URL route for this. But let me know if there is another way I should take care of.

@JoseLuisGJ JoseLuisGJ marked this pull request as ready for review November 6, 2024 13:14
@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 6, 2024

@JoseLuisGJ JoseLuisGJ marked this pull request as draft November 6, 2024 14:35
@elasticmachine
Copy link
Contributor

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!

This reverts commit c64ca3f, reversing
changes made to 5e57f28.
@JoseLuisGJ JoseLuisGJ changed the title [Search][ES3][Coming soon pages] Connectors and Web crawlers coming soon pages Obsolete PR Nov 7, 2024
@sphilipse sphilipse closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants