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

fixed 4025 - allow setting background via name #4081

Merged

Conversation

donaldpipowitch
Copy link
Contributor

@donaldpipowitch donaldpipowitch commented Aug 27, 2018

Issue: #4025

What I did

I updated the backgrounds addon so I can choose my background by name as well and not just by value.

How to test

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

If you run a storybook which includes the backgrounds addon you can set ?background={valueOrName} to choose the background

If your answer is yes to any of these, please make sure to include it in your PR.

For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

Copy link
Member

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

LGTM 👍 nice change.

@@ -99,8 +99,12 @@ export default class BackgroundPanel extends Component {

// debugger;

if (current && backgrounds.find(bg => bg.value === current)) {
this.updateIframe(current);
const foundBackground = current && backgrounds.find(
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the line 97 ☝️ used to get the background from the url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Afaik this hasn't changed. On line 97 we get the value of the query param background. This value is checked against the name and value of the background. I used decodeURI to convert URI encoded spaces back to a real space for example.

Or did I misunderstand your question? 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, my fault 🤦‍♂️

@igor-dv
Copy link
Member

igor-dv commented Aug 28, 2018

Can you please merge from master, it should fix all the CI things

@donaldpipowitch donaldpipowitch force-pushed the 4025-set-background-with-name branch from 61b4dba to ee89527 Compare August 29, 2018 10:17
@codecov
Copy link

codecov bot commented Aug 29, 2018

Codecov Report

Merging #4081 into master will increase coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4081      +/-   ##
==========================================
+ Coverage   40.57%   40.58%   +0.01%     
==========================================
  Files         469      469              
  Lines        5641     5642       +1     
  Branches      748      749       +1     
==========================================
+ Hits         2289     2290       +1     
  Misses       2984     2984              
  Partials      368      368
Impacted Files Coverage Δ
addons/backgrounds/src/BackgroundPanel.js 93.75% <66.66%> (+0.13%) ⬆️

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 0af6571...cbd7741. Read the comment docs.

@donaldpipowitch
Copy link
Contributor Author

I hope it works now 👌

@igor-dv
Copy link
Member

igor-dv commented Aug 29, 2018

There is also a linter warning

image

@donaldpipowitch donaldpipowitch force-pushed the 4025-set-background-with-name branch from ee89527 to cbd7741 Compare August 29, 2018 13:47
@donaldpipowitch
Copy link
Contributor Author

I wonder why I haven't seen that before 🤔 Should be fixed now.

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