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

Fix crash when invalid args for library polished's functions #6939

Merged
merged 14 commits into from
Jun 13, 2019

Conversation

lonyele
Copy link
Member

@lonyele lonyele commented Jun 2, 2019

Issue: Fixes #6855, #6756, #6135

What I did

Problem

When using some functions from polished such as lighten(...), darken(...), its arguments that can't be converted to RGB throws the error

Commit that possibly introduced this bug

Implementation

  • Check if arguments of those functions are valid(gradient, css variable, calc etc...)
  • Since it is used on different places, I put it into util.js from @storybook/theming (I couldn't find the place to put some util/shared functionalities across packages. If it doesn't look ok, please let me know where to put these kinds of functions)

How to test

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

@vercel
Copy link

vercel bot commented Jun 2, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-lonyele-fix-linear-gradient-crash.storybook.now.sh

shilman
shilman previously requested changes Jun 3, 2019
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Nice!!! Can you call these lighten and darken for consistency's sake?

@vercel vercel bot temporarily deployed to staging June 3, 2019 12:50 Inactive
@lonyele
Copy link
Member Author

lonyele commented Jun 3, 2019

Yeap, I applied your feedbacks~

lib/theming/src/utils.ts Outdated Show resolved Hide resolved
@ndelangen
Copy link
Member

Thanks for contributing @lonyele 🎉

@lonyele
Copy link
Member Author

lonyele commented Jun 4, 2019

Oh, I'm working on this one, please wait for a while

@lonyele lonyele changed the title Fix crash when linear-gradient is passed to library polished's functions Fix crash when invalid args for library polished's functions Jun 8, 2019
@lonyele
Copy link
Member Author

lonyele commented Jun 8, 2019

Hm... I'm not sure if the snapshot should be updated(https://circleci.com/gh/storybookjs/storybook/130895?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link) Help is needed here...

I found from library polished that if anything goes wrong, it throws new PolishedError(...) I feel like this is a small hot fix, but it is a not bad start to be safe from polished's exception. Potentially any user inputs that are being passed to polished can go wrong. I'm not sure if it should be refactored since there are a lot and it is only for the few cases, but if it is needed then any custom logic can be added easily here

btw after fixing crash, its values affect normally like this.
Screenshot_2019-06-08 Storybook
To fix these things, I think theming needs to handle more specific parts(maybe relates to)

Copy link
Member Author

@lonyele lonyele left a comment

Choose a reason for hiding this comment

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

@ndelangen ndelangen self-assigned this Jun 12, 2019
@ndelangen
Copy link
Member

ndelangen commented Jun 12, 2019

@lonyele run:

yarn test --core --update

Then commit the changes generated, to fix the snapshots.


I can do it for you, just let me know.

@ndelangen ndelangen modified the milestones: 5.1.x, 5.2.0 Jun 12, 2019
@lonyele
Copy link
Member Author

lonyele commented Jun 13, 2019

@ndelangen Hm... my changes of chromatic looks very wrong. I'll investigate this

@ndelangen
Copy link
Member

Indeed, I denied the changes that seems wrong.

@lonyele
Copy link
Member Author

lonyele commented Jun 13, 2019

@ndelangen oh god... one of the most stupid mistake I've made in few months. I hung my head in shame...
Just a side note, because of this try/catch tests were all passed even with this stupid mistake.

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.

Gradients in theme break shortcut menu
3 participants