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

List providers #45535

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

List providers #45535

wants to merge 8 commits into from

Conversation

dauinh
Copy link
Contributor

@dauinh dauinh commented Jan 10, 2025

related: #43708

Screenshot 2025-01-09 at 4 45 15 PM

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Jan 10, 2025
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Nice.

Just a few nit / suggestions.

airflow/ui/src/pages/Provider.tsx Outdated Show resolved Hide resolved
airflow/ui/src/pages/Provider.tsx Outdated Show resolved Hide resolved
import { useProviderServiceGetProviders } from "openapi/queries";

const embedLinks = (text: string) => {
const regexUrl = /https?:\/\/[^\s)]+/gu;
Copy link
Member

Choose a reason for hiding this comment

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

Where is the regexUrl coming from ? It seems really simple compared to what we have at other places:

  const urlRegex =
    /http(s)?:\/\/[\w.-]+(\.?:[\w.-]+)*([/?#][\w\-._~:/?#[\]@!$&'()*+,;=.%]+)?/g;

airflow/ui/src/pages/Provider.tsx Outdated Show resolved Hide resolved
@bbovenzi
Copy link
Contributor

Could you run pnpm lint && pnpm format locally? Looks like static checks are failing.

@dauinh
Copy link
Contributor Author

dauinh commented Jan 24, 2025

I just did! Thank you for your reviews! I simplified the code by changing the URL into a link component. I couldn't figure out how to work with replace or other JS functions, so I kept embedLinks. If you have any suggestion, please let me know.

I couldn't reproduce the key prop error either, but I did have key prop for each table row. I hope that works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants