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

Ability to open additional menu links in same tab (Resolves #275) #278

Merged
merged 9 commits into from
Jan 9, 2019
Merged

Ability to open additional menu links in same tab (Resolves #275) #278

merged 9 commits into from
Jan 9, 2019

Conversation

zablvit
Copy link
Contributor

@zablvit zablvit commented Nov 17, 2018

Signed-off-by: Vitaliy Zabolotskyy zablvit@gmail.com

Which problem is this PR solving?

Resolves #275
Adds option to ui-config, so added link can be opened in the same tab

Short description of the changes

Added corresponding boolean field to ConfigMenuItem and setting link target accordingly.

@zablvit
Ability to open additional menu links in same tab (jaegertracing#275)
48ad897 
Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>
@codecov
Copy link

codecov bot commented Nov 17, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@7a6190e). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #278   +/-   ##
=========================================
  Coverage          ?   82.58%           
=========================================
  Files             ?      141           
  Lines             ?     3152           
  Branches          ?      654           
=========================================
  Hits              ?     2603           
  Misses            ?      435           
  Partials          ?      114
Impacted Files Coverage Δ
packages/jaeger-ui/src/components/App/TopNav.js 95.45% <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 7a6190e...3167785. Read the comment docs.

yurishkuro
yurishkuro reviewed Nov 17, 2018
View reviewed changes
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.

Thanks for contributing! I made a few minor comments.

packages/jaeger-ui/src/components/App/TopNav.test.js Outdated
@@ -30,6 +30,7 @@ describe('<TopNav>', () => {
{
label: 'Twitter',
url: 'https://twitter.com/JaegerTracing',
openInSameTab: true,
Copy link
Member

Choose a reason for hiding this comment

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

does this make a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In renders the nested menu items test, there is assertion if this is passed further.
I tried to make a test for rendered dropdown, but I just don't know, how to do it.

packages/jaeger-ui/src/types/config.js Outdated Show resolved Hide resolved
packages/jaeger-ui/src/components/App/TopNav.test.js Outdated Show resolved Hide resolved
packages/jaeger-ui/src/components/App/TopNav.js Show resolved Hide resolved
@yurishkuro yurishkuro mentioned this pull request Nov 17, 2018
Closed
zablvit and others added 4 commits November 18, 2018 20:02
@zablvit
Add negative test case
8610528 
Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>
@zablvit
Add helper function to create item links
3759d0a 
Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>
@zablvit
Fix no-use-before-define lint error
a4967ac 
Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>
@zablvit
Merge branch 'master' into master
79b7382
tiffon
tiffon suggested changes Nov 30, 2018
View reviewed changes
Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

Great work and thanks for creating this! Two small suggestions.

packages/jaeger-ui/src/components/App/TopNav.js Show resolved Hide resolved
packages/jaeger-ui/src/types/config.js Outdated Show resolved Hide resolved
@tiffon
Use anchorTarget in custom menu configuration
3d08f15 
Signed-off-by: Joe Farro <joef@uber.com>
@ghost ghost assigned tiffon Jan 6, 2019
@ghost ghost added the review label Jan 6, 2019
tiffon
tiffon previously approved these changes Jan 6, 2019
View reviewed changes
@tiffon tiffon dismissed their stale review January 6, 2019 08:45

Inadvertently approved my own changes.

@tiffon tiffon requested a review from everett980 January 6, 2019 08:45
everett980
everett980 approved these changes Jan 7, 2019
View reviewed changes
Copy link
Collaborator

@everett980 everett980 left a comment

Choose a reason for hiding this comment

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

one typo, otherwise seems good

packages/jaeger-ui/src/components/App/TopNav.test.js Outdated Show resolved Hide resolved
tiffon and others added 2 commits January 9, 2019 09:51
@tiffon
Fix typo in test case
f810187 
Signed-off-by: Joe Farro <joef@uber.com>
@tiffon
Merge branch 'master' into master
3167785
@tiffon tiffon merged commit d61767a into jaegertracing:master Jan 9, 2019
@ghost ghost removed the review label Jan 9, 2019
everett980 pushed a commit to everett980/jaeger-ui that referenced this pull request Jan 16, 2019
@zablvit @everett980
Ability to open additional menu links in same tab (Resolves jaegertra…
f08a609 
…cing#275) (jaegertracing#278)

* Ability to open additional menu links in same tab (jaegertracing#275)

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Add negative test case

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Add helper function to create item links

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Fix no-use-before-define lint error

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Use anchorTarget in custom menu configuration

Signed-off-by: Joe Farro <joef@uber.com>

* Fix typo in test case

Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>
@yurishkuro yurishkuro mentioned this pull request Aug 10, 2019
Closed
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
@zablvit @tiffon
Ability to open additional menu links in same tab (Resolves jaegertra…
b2c2c97 
…cing#275) (jaegertracing#278)

* Ability to open additional menu links in same tab (jaegertracing#275)

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Add negative test case

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Add helper function to create item links

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Fix no-use-before-define lint error

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Use anchorTarget in custom menu configuration

Signed-off-by: Joe Farro <joef@uber.com>

* Fix typo in test case

Signed-off-by: Joe Farro <joef@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
@zablvit @everett980
Ability to open additional menu links in same tab (Resolves jaegertra…
9223cf7 
…cing#275) (jaegertracing#278)

* Ability to open additional menu links in same tab (jaegertracing#275)

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Add negative test case

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Add helper function to create item links

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Fix no-use-before-define lint error

Signed-off-by: Vitaliy Zabolotskyy <zablvit@gmail.com>

* Use anchorTarget in custom menu configuration

Signed-off-by: Joe Farro <joef@uber.com>

* Fix typo in test case

Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Everett Ross <reverett@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

@everett980 everett980 everett980 approved these changes

@tiffon tiffon tiffon left review comments

Assignees

@tiffon tiffon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zablvit @tiffon @everett980 @yurishkuro