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

Add boolean property to config #1521

Conversation

Binrix
Copy link
Contributor

@Binrix Binrix commented Jun 27, 2023

This pull request resolves the issue #1211

This property is later used to prevent the popup of a new page. When set to true, it will alter all _blank targets for links to empty. A utils function is provided.

This property is later used to prevent the popup of a
new page. When set to true, it will alter all _blank targets
for links to empty. A utils function is provided.

Signed-off-by: Benjamin Klein <benjamin.klein.bk1@roche.com>
@Binrix
Copy link
Contributor Author

Binrix commented Jun 27, 2023

As discussed before, I added a utils function that checks if the boolean property in the config is set to true or false and sets the target of a link to _blank, _top or empty.

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (1cad58c) 95.93% compared to head (90d3866) 95.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1521      +/-   ##
==========================================
+ Coverage   95.93%   95.96%   +0.02%     
==========================================
  Files         241      242       +1     
  Lines        7553     7557       +4     
  Branches     1982     1984       +2     
==========================================
+ Hits         7246     7252       +6     
+ Misses        307      305       -2     
Impacted Files Coverage Δ
...components/SearchTracePage/SearchResults/index.tsx 86.15% <ø> (ø)
...ts/TraceDiff/TraceDiffHeader/TraceTimelineLink.tsx 100.00% <ø> (ø)
...nents/TracePage/TracePageHeader/AltViewOptions.tsx 100.00% <ø> (ø)
...ents/TracePage/TracePageHeader/TracePageHeader.tsx 97.56% <ø> (ø)
...i/src/components/TracePage/TraceSpanView/index.tsx 89.39% <ø> (ø)
...ackages/jaeger-ui/src/constants/default-config.tsx 75.00% <ø> (ø)
.../SearchTracePage/SearchResults/ResultItemTitle.tsx 100.00% <100.00%> (ø)
packages/jaeger-ui/src/utils/config/get-target.tsx 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -88,7 +89,7 @@ export default function AltViewOptions(props: Props) {
<Link
to={prefixUrl(`/api/traces/${traceID}?prettyPrint=true`)}
rel="noopener noreferrer"
target="_blank"
target={getTargetBlankOrTop()}
Copy link
Member

Choose a reason for hiding this comment

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

why did you not think EmptyOrBlank would work 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.

The problem is that when the target is empty, the json will be shown in the same window and the user cannot return to the page before (at least on the instrument because it has no keyboard or possibility to reach a return button from the browser). This is why I decided to use top: when the target is top the user can still access the navigation bar and go back.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm

Binrix and others added 3 commits June 27, 2023 15:47
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Benjamin Klein <45211351+Binrix@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Benjamin Klein <45211351+Binrix@users.noreply.github.com>
@yurishkuro yurishkuro enabled auto-merge (squash) June 27, 2023 13:53
@yurishkuro yurishkuro merged commit 3abc200 into jaegertracing:main Jun 27, 2023
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.

2 participants