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

UI: open brandUrl in new window #6106

Merged

Conversation

toshi1127
Copy link
Contributor

@toshi1127 toshi1127 commented Mar 15, 2019

Issue: #6070

What I did

Opened brandUrl in new window when the brand image was clicked

How to test

  • Is this testable with Jest or Chromatic screenshots? Yes. I updated the snapshot.
  • Does this need a new example in the kitchen sink apps? No.
  • Does this need an update to the documentation? No.

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

Merging #6106 into next will decrease coverage by 1.31%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next    #6106      +/-   ##
=========================================
- Coverage   37.1%   35.79%   -1.32%     
=========================================
  Files        648      648              
  Lines       9524     9521       -3     
  Branches    1381     1352      -29     
=========================================
- Hits        3534     3408     -126     
- Misses      5411     5536     +125     
+ Partials     579      577       -2
Impacted Files Coverage Δ
lib/ui/src/components/sidebar/SidebarHeading.js 88.88% <100%> (+0.42%) ⬆️
addons/a11y/src/components/Report/index.tsx 0% <0%> (-100%) ⬇️
addons/a11y/src/constants.ts 0% <0%> (-100%) ⬇️
addons/a11y/src/components/A11YPanel.tsx 0% <0%> (-94.45%) ⬇️
addons/a11y/src/components/Tabs.tsx 0% <0%> (-90.48%) ⬇️
addons/a11y/src/components/Report/Info.tsx 0% <0%> (-85.72%) ⬇️
addons/a11y/src/components/Report/Item.tsx 0% <0%> (-85%) ⬇️
addons/a11y/src/components/Report/Tags.tsx 0% <0%> (-71.43%) ⬇️
addons/a11y/src/components/Report/Rules.tsx 0% <0%> (-66.67%) ⬇️
addons/a11y/src/components/Report/Elements.tsx 0% <0%> (-63.64%) ⬇️
... and 7 more

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 fa04c38...d7af6f9. Read the comment docs.

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

I like it! one feature-request and then this will get merged:

Please only add the target when the url is an external link.

@toshi1127
Copy link
Contributor Author

@ndelangen
only added the target when the url is an external link.
Please confirm!!

@ndelangen
Copy link
Member

If target is an empty string, target="" I guess nothing special happens.

Not having the prop at all seems a bit cleaner to me, but this is good to merge IMHO!

Good addition @toshi1127 👍

@ndelangen ndelangen merged commit c13a353 into storybookjs:next Mar 15, 2019
@shilman shilman added this to the v5.1.0 milestone Mar 15, 2019
@shilman shilman changed the title open brandUrl in new window UI: open brandUrl in new window Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants