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 missing animations #671

Merged

Conversation

johannchopin
Copy link
Collaborator

@johannchopin johannchopin commented Dec 30, 2020

Description

Since the styles are pure css it's no more possible to have the previous animation for the gitmoji codes (not possible to easily have a pure css declaration for rgba($color, .7)). So I decided to use the same animation that the one in the gitmoji-browser-extension (animated underline on :hover).

It still in WIP since flow trigger some errors like:

Error ┈┈┈┈┈┈┈┈┈┈┈┈node_modules/.staging/@ampproject/toolbox-optimizer-a8ba599c/package.json:1:1

Unexpected end of input, expected a valid JSON value

So I remove the checking part to open the CR. Has someone an idea how to fix it?

Tests

  • All tests passed.

Close #670

@vercel
Copy link

vercel bot commented Dec 30, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carloscuesta/gitmoji/m9nasph3x
✅ Preview: https://gitmoji-git-fork-johannchopin-feature-fix-missing-animations.carloscuesta.vercel.app

src/utils/theme/theme.css Outdated Show resolved Hide resolved
src/utils/theme/theme.css Outdated Show resolved Hide resolved
@carloscuesta
Copy link
Owner

Hey @johannchopin the flow error you're pointing to seems something about cache on your local environment. If you take a look at the pipeline the tests are passing, so try to stop and start the flow server and try again

Copy link
Owner

@carloscuesta carloscuesta left a comment

Choose a reason for hiding this comment

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

Cool! 👏🏼

package.json Outdated Show resolved Hide resolved
src/components/GitmojiList/Gitmoji/index.js Outdated Show resolved Hide resolved
src/utils/theme/theme.css Outdated Show resolved Hide resolved
src/utils/theme/theme.css Outdated Show resolved Hide resolved
@vhoyer
Copy link
Collaborator

vhoyer commented Dec 30, 2020

Donno what to say about the flow besides: it works in my machine 😅 have you tried running npm i?

@johannchopin
Copy link
Collaborator Author

Donno what to say about the flow besides: it works in my machine 😅 have you tried running npm i?

Yes I just delete my node_modules and install again and everything was working 👍

src/utils/theme/theme.css Outdated Show resolved Hide resolved
src/utils/theme/theme.css Outdated Show resolved Hide resolved
@carloscuesta
Copy link
Owner

Hey @johannchopin would you like some help to get this finished?

@johannchopin
Copy link
Collaborator Author

@carloscuesta Thanks for your help I updated your code to have an underline animation 👍 Does it look good for you?

@carloscuesta
Copy link
Owner

Yeah! It looks awesome, the underline effect is so much better ❤️

Copy link
Owner

@carloscuesta carloscuesta left a comment

Choose a reason for hiding this comment

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

I think we are ready to ship this! 🚀

@carloscuesta carloscuesta changed the title WIP: 🩹 Fix missing animations 🩹 Fix missing animations Jan 14, 2021
@vhoyer
Copy link
Collaborator

vhoyer commented Jan 14, 2021

please, allow me to propose a change to this :D

@carloscuesta
Copy link
Owner

Sure @vhoyer!

@vhoyer
Copy link
Collaborator

vhoyer commented Jan 14, 2021

As I couldn't open a PR for this, for whatever reason, feel free to remove those two last commits of mine ❤️

@carloscuesta
Copy link
Owner

I see, your solution is a little bit simpler on the code but It relies on using inline styles, I think I prefer using classNames in here 👀

@vhoyer
Copy link
Collaborator

vhoyer commented Jan 14, 2021

Really? Oh, danm, I mean, inline styles are not a sin, 😂 and I really prefer using inline styles to embedding them on the component, like the previous solution, but I guess this comes down to taste, so again, feel free to remove my last two commits if you two don't agree with my changes 😄

@carloscuesta
Copy link
Owner

No problem at all! We can leave it like this, just a style preference 👍🏼

Copy link
Owner

@carloscuesta carloscuesta left a comment

Choose a reason for hiding this comment

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

Let's go! 🚀

@vhoyer
Copy link
Collaborator

vhoyer commented Jan 14, 2021

besides, I think the major drawback of using inline-styles in general is increasing the specificity of the style without giving a way for other classes to override it, which leads to bad reusability, but when it comes to using only css-variables in the inline-style, this problem kinda fades away once, we are only registering a token for the classes to use :D

EDIT: just commenting this to give a base to my line of thought haha

@carloscuesta
Copy link
Owner

carloscuesta commented Jan 14, 2021

Yes! That's why I think we can use the inline styles approach! It's simpler and cleaner ✨ . Not a big difference in here using classNames vs the inline defined variable so 🚀

@carloscuesta carloscuesta merged commit f70f6fc into carloscuesta:master Jan 14, 2021
@carloscuesta
Copy link
Owner

carloscuesta commented Jan 14, 2021

We did a great collaboration in here! Thanks @vhoyer @johannchopin ❤️ 💪🏼 👏🏼

@johannchopin johannchopin deleted the feature/fix-missing-animations branch January 14, 2021 20:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Add :hover animation on emoji and code
3 participants