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 adblocker and 0/single node disclaimers #502

Conversation

everett980
Copy link
Collaborator

@everett980 everett980 commented Dec 18, 2019

Which problem is this PR solving?

  • DDG request is blocked by some adblockers
  • Plexus does not render single vertex

Short description of the changes

  • Add disclaimer to whitelist Jaeger on DDG request failure details view.
    image
    ("New!" banner not in diff)
  • Add disclaimer when there are zero or fewer vertices to view.
    No dependencies found:
    image
    Dependencies found but hidden:
    image

This PR is branched off of another PR, this commit contains the diff for this PR

@codecov
Copy link

codecov bot commented Dec 18, 2019

Codecov Report

Merging #502 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
+ Coverage   92.87%   92.89%   +0.01%     
==========================================
  Files         197      197              
  Lines        4789     4797       +8     
  Branches     1155     1159       +4     
==========================================
+ Hits         4448     4456       +8     
  Misses        300      300              
  Partials       41       41
Impacted Files Coverage Δ
...aeger-ui/src/components/DeepDependencies/index.tsx 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 135ccfe...0be2f7a. Read the comment docs.

@yurishkuro
Copy link
Member

there seems to be many changes not related to just disclaimers

@everett980
Copy link
Collaborator Author

@yurishkuro the PR is branched off of another PR. There is a link in the PR description to the commit that contains the disclaimer changes.

@everett980
Copy link
Collaborator Author

The middle branch was merged, so the diff is good now (except for two files with TODO delete changes comments that I definitely deleted on the middle branch.....)

@@ -20,6 +20,10 @@ export default deepFreeze(
Object.defineProperty(
{
archiveEnabled: false,
// TODO: remove default
deepDependencies: {
menuEnabled: true,
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want that in oss? There's no backend to generate DDGs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah hence the TODO and comment above that I thought TODOs were dealt with...
definitely won't have this on by default. same with the New! css

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Everett Ross <[email protected]>
@everett980 everett980 merged commit 537bbe2 into jaegertracing:master Jan 6, 2020
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Temp: Default link visible with banner

Signed-off-by: Everett Ross <[email protected]>

* WIP: Use server ops in ddg, track conv&collapse

Signed-off-by: Everett Ross <[email protected]>

* Add tests

Signed-off-by: Everett Ross <[email protected]>

* Add disclaimers for (near) empty graph&adblocker

Signed-off-by: Everett Ross <[email protected]>

* Remove default enabled

Signed-off-by: Everett Ross <[email protected]>

Signed-off-by: vvvprabhakar <[email protected]>
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