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

fade-in-bloom and fade-out-boom for #255 #260

Merged
merged 3 commits into from Aug 11, 2022

Conversation

hchiam
Copy link
Contributor

@hchiam hchiam commented Aug 10, 2022

demo: https://codepen.io/hchiam/pen/bGvjoev
(hey, sorry for the delay, got distracted earlier today)

Copy link
Contributor Author

@hchiam hchiam left a comment

Choose a reason for hiding this comment

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

Ready for review. Not sure if 1086 needs to be edited in the test file:
https://github.com/argyleink/open-props/runs/7760228885?check_suite_focus=true#step:7:32

test('Should have an all included import', t => {
  t.is(Object.keys(OpenProps).length, 1086)
})

Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

awesome, looks good! thanks a bunch, this'll be a cool addition. bloom-out looks nice, great work.

todo:

  • update docsite/index.html animations section to have a demo
  • update docsite "The Props" section with the bloom fades
  • update the test to account for the new props (1094)

I feel like only the test update is required of you, I can update the docsite. But if you're down to tackle the docsite additions I'd be happy to merge it all in one PR 🙂

@hchiam
Copy link
Contributor Author

hchiam commented Aug 11, 2022

agreed, i'll update the count in the test, and i'll leave the docsite update to you @argyleink

@hchiam
Copy link
Contributor Author

hchiam commented Aug 11, 2022

i can confirm that the +8 in the count (from 1086 to 1094) is coming solely from the changes in this PR, because when i revert (and run yarn bundle; yarn test), the test passes:
image
What confuses me is why there's +8 instead of just +4

EDIT: i see now, it's double what i expected because it generates both CSS and JS keys:

  1. '--animation-fade-in-bloom'
  2. '--animation-fade-in-bloom-@'
  3. '--animation-fade-out-bloom'
  4. '--animation-fade-out-bloom-@'
  5. animationFadeInBloom
  6. 'animationFadeInBloom@'
  7. animationFadeOutBloom
  8. 'animationFadeOutBloom@'

@hchiam
Copy link
Contributor Author

hchiam commented Aug 11, 2022

Ready for review. Updated test. I'll leave the docsite update to you, thanks!

Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

🤘🏻

@argyleink argyleink merged commit 79dca04 into argyleink:main Aug 11, 2022
@argyleink
Copy link
Owner

just pushed updates to the docsite and published a new OP version 👍🏻

i'm curious if you think there's a nice way to make the brightness light/dark contextual? like in a dark theme instead of blooming white it blooms grey or black? i tried a couple things, seems like it could work nice, but didnt find a happy spot for it. thoughts?!

@hchiam
Copy link
Contributor Author

hchiam commented Aug 11, 2022 via email

@argyleink
Copy link
Owner

yep! shadows does it, can follow that path. but yeah, if you find a great way to make it adaptive to color-scheme then we can deploy an upgraded version 🙂

@hchiam
Copy link
Contributor Author

hchiam commented Aug 11, 2022

oh, make the bloom light/dark dependent, not simply on dark mode, but on any color-scheme, eh? very interesting. i might not be able to work on this until next week, so feel to work on it meanwhile

kelvindecosta pushed a commit to kelvindecosta/open-props that referenced this pull request Nov 16, 2022
* fade-in-bloom and fade-out-boom for argyleink#255

* update test 1086 + 8 = 1094

* remove console log (used for quick debug)
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.

None yet

2 participants