Skip to content

Conversation

IND-Anshuman
Copy link
Contributor

@IND-Anshuman IND-Anshuman commented Jun 17, 2025

Overview

  1. This PR fixes [Feature Request]: Make a YDO animation component. #7.
  2. This PR does the following: [Adds a reusable animation component made by using a JSON file and rendered using the lottie-react player]

Essential Checklist

  • The PR title starts with "Fix #bugnum:7 ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
  • The PR is made from a branch that's not called "main/master".

Proof that changes are correct

<!

Recording.2025-06-17.105837.mp4

--
Add videos/screenshots of the user-facing interface to demonstrate that the changes
made in this PR work correctly. If this PR is for a developer-facing feature,
provide the videos/screenshots of developer-facing interface.

Please also include videos/screenshots of the developer tools
browser console, so that we can be sure that there are no console errors.

If the changes in your PRs are autogenerated via a script and you cannot provide proof
for the changes then please leave a comment "No proof of changes needed because {{Reason}}"
and remove all the sections below.
-->

PR Pointers

  • If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL".
  • Never force push. If you do, your PR will be closed.

@Zapper9982
Copy link
Collaborator

Hi @IND-Anshuman the animation is perfect but can u just look into it that only the animation exists theres no background to it , cause this will be integrated in many part of the website so the background wont be necessary

@Zapper9982
Copy link
Collaborator

also @IND-Anshuman can u integrate it with the Landing page , with the latest commit perhaps ?

Copy link
Collaborator

@Zapper9982 Zapper9982 left a comment

Choose a reason for hiding this comment

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

Added some changes required do look into it

autoplay = true,
speed = 1,
onClick = null,
style = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the styling please use a separate .css file inside the same folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

Copy link
Contributor Author

@IND-Anshuman IND-Anshuman Jun 17, 2025

Choose a reason for hiding this comment

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

should i replace the ydo logo with the animation in the landing page.
if both the the animation and the logo is needed then i have to change the size and dimensions of the ydo logo and the start button. If a design can be made it would be helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on replacing the logo it will look like this

Recording.2025-06-17.184441.mp4

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks epic , @Om-Thorat requires any changes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I make the changes in the landing page(changing css components ,media query etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to adjust the logo for small devices ,but it goes a bit left-right at some point of time. So I tried to make it symmetric for the common mobile screen resolutions.

Screen.Recording.2025-06-19.205517.mp4

Copy link
Contributor

@Om-Thorat Om-Thorat Jun 19, 2025

Choose a reason for hiding this comment

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

Yes, go on make the changes in the landing page since this is a medium issue it's only justified you put it on the landing page. Please refer to the PR made by abhay for the landing page sync your local with head. and as suggested by @Zapper9982 put css in a different file.

it goes a bit left-right

That shouldnt happen probably a problem with css can you please debug it. Look into use flex, justify and align properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I tried using transform (translate) and it worked fine.

Screen.Recording.2025-06-20.132050.1.mp4

@Zapper9982 Zapper9982 requested a review from Om-Thorat June 17, 2025 12:48
Copy link
Contributor

@Om-Thorat Om-Thorat left a comment

Choose a reason for hiding this comment

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

Good Job so far Anshuman, just tiny changes

@@ -0,0 +1,33 @@
import Lottie from 'lottie-react';
import YDOanimation from '../../assets/Animation/YDOanimation.json';
const YDOAnimation = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

change the name to something else please. The two variable names here are conflicting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

did you try converting this one to .lottie and check if it worked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the present one is done using the lottie file. The problem was that vite was trying to parse the lottie file as a js file ,so I declared the .lottie file as an asset in vite config.js and it worked. Though the animation using the lottie file is looking a bit glitchy and is taking more time in rendering.

Recording.2025-06-20.134012.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Zapper9982 , @Om-Thorat , while using the lottie file, the animation is getting glitchy and taking more time to render, so should I stick to the JSON file or try using another lottie player. This time I used @lottiefiles/dotlottie-react

Copy link
Contributor Author

Choose a reason for hiding this comment

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

React-lottie player does not works on .lottie file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lottie-web, lottie-react, react-lottie only supports JSON file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Zapper9982, @Om-Thorat ,I have done the specified changes (removed the unnecessary props, changed the names of conflicting variables, added typescript to dev. dependency), as for the animation player I have used lottie-react(using JSON file) since I am not able to resolve the slow rendering problem while using the .lottie file. Should I commit the changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure go for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen.Recording.2025-06-26.162508.mp4

speed = 1,
onClick = null,
style = {
width: '50vw',
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure these parameters are correct. Try to have as minimal styling as possible within the component so that it can be modified externally and used accross pages like navbar, landing and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got, it. I will remove the unnecessary props.

@Zapper9982
Copy link
Collaborator

image @IND-Anshuman took a screenshot from your video recording , can we perhaps have the YDO Animation + the text + Getstarted button centered into the Blue section of the page

@Zapper9982
Copy link
Collaborator

@IND-Anshuman can u fix the build Issue too

@IND-Anshuman
Copy link
Contributor Author

image @IND-Anshuman took a screenshot from your video recording , can we perhaps have the YDO Animation + the text + Getstarted button centered into the Blue section of the page

Ok , I will try

@IND-Anshuman
Copy link
Contributor Author

Is it fine?

Uploading Recording 2025-06-29 113359 (1).mp4…

@Om-Thorat
Copy link
Contributor

Is it fine?

Uploading Recording 2025-06-29 113359 (1).mp4…

seems like u forgot to attach the video!

@IND-Anshuman
Copy link
Contributor Author

Ahh ,I made the comment while the vedio was loading.

Recording.2025-06-29.113359.1.mp4

@Zapper9982
Copy link
Collaborator

Hi @IND-Anshuman looks good to me apart from that can we increase the size of the animation a bit , cause the page looks empty otherwise . Other than that we can go ahead and merge after you do it

Copy link
Collaborator

@Zapper9982 Zapper9982 left a comment

Choose a reason for hiding this comment

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

@IND-Anshuman Why are there 4 .animation in the CSS

.animation {
scale: 1.2;
position: relative;
top: 50%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Umm are 4 .animation really required ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are for different screen resolution .The landing page css had 4 media queries so I added the .animation in each one otherwise the animation won't allign with the get started button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and yes I will try to increase the size of the animation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this fine

Recording.2025-07-04.134911.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Zapper9982 just tell if there are any more modifications ,I will do it by today

@Zapper9982 Zapper9982 merged commit 61b3a17 into bsoc-bitbyte:main Jul 4, 2025
2 checks passed
@Zapper9982 Zapper9982 added enhancement New feature or request Difficulty : Medium medium level issue BSoC'25 Created for BSoC'25 Frontend labels Jul 4, 2025
@IND-Anshuman IND-Anshuman deleted the IND_Anshuman branch July 4, 2025 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BSoC'25 Created for BSoC'25 Difficulty : Medium medium level issue enhancement New feature or request Frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Make a YDO animation component.
3 participants