Skip to content

chore: Convert EuiCustomLink to TypeScript in /ui#5059

Merged
franciscojavierarceo merged 1 commit into
feast-dev:masterfrom
peruukki:ui-eui-custom-link-tsx
Mar 1, 2025
Merged

chore: Convert EuiCustomLink to TypeScript in /ui#5059
franciscojavierarceo merged 1 commit into
feast-dev:masterfrom
peruukki:ui-eui-custom-link-tsx

Conversation

@peruukki

@peruukki peruukki commented Feb 15, 2025

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

EuiCustomLink was the last JavaScript component, convert it to TypeScript too.

Which issue(s) this PR fixes:

No related issue.

Misc

The href prop was never used, so we omit it from the props type definition, and no longer pass it to the component. (We define href in EuiCustomLink based on the passed to prop.)

We could also have chosen to drop the to prop and use href instead, but to is used by React Router's Link, and it makes sense to use the same name in EuiCustomLink since we pass it to React Router's navigate function; the naming hints that it's tied to React Router and not just a regular URL path.

These changes partially overlap with #5004, but I'll remove such changes from the other PR if this gets merged first, and vice versa.

@peruukki peruukki requested a review from a team as a code owner February 15, 2025 08:32
This was the last JavaScript component, convert it to TypeScript too.

The `href` prop was never used, so we omit it from the props type
definition, and no longer pass it to the component.

We could also have chosen to drop the `to` prop and use `href` instead,
but `to` is used by React Router's Link
(https://reactrouter.com/6.29.0/components/link), and it makes sense to
use the same name in EuiCustomLink since we pass it to React Router's
`navigate` function; the naming hints that it's tied to React Router and
not just a regular URL path.

Signed-off-by: Harri Lehtola <peruukki@hotmail.com>
@peruukki

peruukki commented Mar 1, 2025

Copy link
Copy Markdown
Contributor Author

This is still waiting for a review, would someone be available for it? 🙏

@franciscojavierarceo franciscojavierarceo 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.

lgtm.

@franciscojavierarceo franciscojavierarceo merged commit 244da16 into feast-dev:master Mar 1, 2025
@franciscojavierarceo

Copy link
Copy Markdown
Member

@peruukki feel free to tag me directly next time! Sorry for the delay there! Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants