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.

Sign up for GitHub

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

Jump to bottom

MINOR: open links in new tab in DataAssetsHeader #18172

Merged
merged 3 commits into from
Oct 9, 2024
Merged

MINOR: open links in new tab in DataAssetsHeader #18172

merged 3 commits into from
Oct 9, 2024

Conversation

fuzmish
Copy link
Contributor

@fuzmish fuzmish commented Oct 8, 2024

Describe your changes:

(No issue opened explicitly)

  • What changes did you make?
    • Change the links in the DataAssetsHeader component to open in a new tab.
  • Why did you make them?
    • Some entities have properties with link information to external web resources (e.g., Dashboard.sourceUrl), which may be rendered as <a> links in the DataAssetsHeader component.
    • Most or all of these are resources in a separate location from the OpenMetadata UI, and it is preferred that the links be opened in a new tab. (Currently, they open in the same tab.)
  • How did you test your changes?
    • I have added the unit test for the ExtraInfoLink component.
    • I have tested the behavior in the local development UI server.

References:

Type of change:

  • Improvement

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • I have added tests around the new logic.
  • For connector/ingestion changes: I updated the documentation.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 8, 2024

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 8, 2024

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

chirag-madlani
chirag-madlani reviewed Oct 8, 2024
View reviewed changes
openmetadata-ui/src/main/resources/ui/src/utils/DataAssetsHeader.utils.tsx Outdated
href={mlModelDetail.server}
label={t('label.server')}
value={mlModelDetail.server}
/>
)}
{mlModelDetail.dashboard && (
<ExtraInfoLink
newTab
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't be new tab, it's internal route

openmetadata-ui/src/main/resources/ui/src/utils/DataAssetsHeader.utils.tsx Show resolved Hide resolved
openmetadata-ui/src/main/resources/ui/src/utils/DataAssetsHeader.utils.tsx Show resolved Hide resolved
Sachin-chaurasiya
Sachin-chaurasiya reviewed Oct 8, 2024
View reviewed changes
Copy link
Member

@Sachin-chaurasiya Sachin-chaurasiya left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @fuzmish, left one comment and rest looks good.

openmetadata-ui/src/main/resources/ui/src/utils/DataAssetsHeader.utils.tsx Outdated
href={mlModelDetail.server}
label={t('label.server')}
value={mlModelDetail.server}
/>
)}
{mlModelDetail.dashboard && (
<ExtraInfoLink
newTab
Copy link
Member

Choose a reason for hiding this comment

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

@fuzmish, I believe this is an internal path, so we don’t need the newTab as true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment. a4bd542 I've fixed it.

@Sachin-chaurasiya Sachin-chaurasiya added the safe to test Add this label to run secure Github workflows on PRs label Oct 8, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 8, 2024

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
 64.18% (37781/58869) 40.95% (15077/36818) 42.95% (4551/10595)

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Oct 8, 2024

Quality Gate Passed Quality Gate passed for 'open-metadata-ui'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@chirag-madlani chirag-madlani merged commit dc24c00 into open-metadata:main Oct 9, 2024
16 checks passed
harshach pushed a commit that referenced this pull request Oct 19, 2024
@fuzmish @harshach
MINOR: open links in new tab in DataAssetsHeader (#18172)
19152f9 
* MINOR: open links in new tab in DataAssetsHeader

* fix: make mlModelDetail.dashboard internal link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sachin-chaurasiya Sachin-chaurasiya Sachin-chaurasiya left review comments

@chirag-madlani chirag-madlani chirag-madlani approved these changes

@ShaileshParmar11 ShaileshParmar11 Awaiting requested review from ShaileshParmar11

@karanh37 karanh37 Awaiting requested review from karanh37

Assignees
No one assigned
Labels
safe to test Add this label to run secure Github workflows on PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fuzmish @chirag-madlani @Sachin-chaurasiya