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

Added Animated Hamburger Menu #488

Conversation

sushantgwr87
Copy link
Contributor

@sushantgwr87 sushantgwr87 commented Oct 2, 2022

Animated Hamburger Menu Pull Request

What does this PR do?

Fixes #245
Link to the issue

Description

Added styled component with animation for mobile header burger menu. Removed svg icons used for it as well.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@vercel
Copy link

vercel bot commented Oct 2, 2022

@sushantgwr87 is attempting to deploy a commit to the Meili Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Oct 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
landing ✅ Ready (Inspect) Visit Preview Oct 10, 2022 at 3:03PM (UTC)

@bidoubiwa
Copy link
Contributor

Hey @sushantgwr87 thanks so much for you contribution 🙏
Could you fix the lining issues ? Its hard to read the code between the errors.

yarn lint

@bidoubiwa bidoubiwa self-requested a review October 5, 2022 11:35
@sushantgwr87
Copy link
Contributor Author

@bidoubiwa Updated the Code and Removed Errors. Kindly Check Once Again.

@bidoubiwa
Copy link
Contributor

I still see linting errors 😔 See https://github.com/meilisearch/landing/pull/488/files

@sushantgwr87
Copy link
Contributor Author

Yeah, Actually my system shows different lint errors and here bit more. No problem I will fix all (this time both check) and will let you know.

@sushantgwr87
Copy link
Contributor Author

Sir Kindly run the checks again, this time I guess errors should be gone. Before I was not using prettier extension that's why it linted different errors in VS code.

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Comment on lines 103 to 108
<DialogDisclosure {...dialog} style={{ color: 'white', height: '24px' }}>
<MenuBtnWrapper clicked={dialog.visible}>
<MenuBtnBar />
<MenuBtnBar />
<MenuBtnBar />
</MenuBtnWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you create a Hamburger component out of it and add it in an apart 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.

Sure. working on it. I will create file in same folder regarding it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefered you add it in the components folder.

@sushantgwr87
Copy link
Contributor Author

I made it. Check once if its correct or not.

I created Hamburger Taking whole DialogDisclosure part in it and then importing the hamburger in MobileHeader.js.

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

You can leave DialogDisclosure in MobileHeader.js

@sushantgwr87
Copy link
Contributor Author

You can leave DialogDisclosure in MobileHeader.js

Okay Updating it.

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

Could you add the Hamburger in the components folder ?

@sushantgwr87
Copy link
Contributor Author

Could you add the Hamburger in the components folder ?

Yes Updating it

@sushantgwr87
Copy link
Contributor Author

Updated Sir, Check Once.

@bidoubiwa
Copy link
Contributor

Hey @sushantgwr87

Since @mdubus is not present, I can not be sure that everything is as wanted. So unfortunately, we'll have to wait until next week to approve or not your pr.

These are the questions that I'm thinking about:

  • The stories should be updated
  • The unused icons should be removed
  • Should the animation be done with lottie as mentioned in the issue?
  • In what folder should the component be ? As maybe I'm wrong

@sushantgwr87
Copy link
Contributor Author

Hey @sushantgwr87

Since @mdubus is not present, I can not be sure that everything is as wanted. So unfortunately, we'll have to wait until next week to approve or not your pr.

These are the questions that I'm thinking about:

  • The stories should be updated
  • The unused icons should be removed
  • Should the animation be done with lottie as mentioned in the issue?
  • In what folder should the component be ? As maybe I'm wrong

No Problem. Will See it on next Week.

@mdubus
Copy link
Member

mdubus commented Oct 10, 2022

Hello @sushantgwr87 👋

Thanks a lot for your PR and sorry for the inconvenience of me not being there during the first week of Hacktoberfest.

Is there a reason why you chose to create a custom Component for this instead of using Lottie as recommended in the issue?

Thanks a lot and have a good day ✨

@sushantgwr87
Copy link
Contributor Author

Hello @sushantgwr87 👋

Thanks a lot for your PR and sorry for the inconvenience of me not being there during the first week of Hacktoberfest.

Is there a reason why you chose to create a custom Component for this instead of using Lottie as recommended in the issue?

Thanks a lot and have a good day ✨

@mdubus No Sir. There is no specific reason for not using Lottie file for this. Actually we are already using styled components so I thought this animation can be easily created without much fuss and with little bit of css animation, it will work as needed.

Though If you think I should use Lottie file for it, I can work on that and create it using that.

@mdubus
Copy link
Member

mdubus commented Oct 10, 2022

Though If you think I should use Lottie file for it, I can work on that and create it using that.

Thanks a lot for your answer 🙏 Indeed it would be great if you could use Lottie instead, for multiple reasons:

  • I see a little CSS problem inside your animation, which would be fixed with Lottie
  • It will be easier to maintain as every animation will be in the same format and in the same place
  • It will require less code

Thanks a lot for your time, and I deeply apologize for the inconvenience 🙏

@sushantgwr87
Copy link
Contributor Author

Though If you think I should use Lottie file for it, I can work on that and create it using that.

Thanks a lot for your answer 🙏 Indeed it would be great if you could use Lottie instead, for multiple reasons:

  • I see a little CSS problem inside your animation, which would be fixed with Lottie
  • It will be easier to maintain as every animation will be in the same format and in the same place
  • It will require less code

Thanks a lot for your time, and I deeply apologize for the inconvenience 🙏

Understood and No Worries. I will ping you once I fix it. Thank you.

@sushantgwr87
Copy link
Contributor Author

sushantgwr87 commented Oct 11, 2022

@mdubus Hello Sir,

Need help in fixing Lottie component or code for animation.

I created Hamburger menu animation using Lottie file and already made Lottie component. Problem is whole animation runs on button click from = to x back to =. (Lottie file has same animation too)

I tried using segments but seems they don't work with this package I guess as it does not shows props relating it or such toggle support. I am still researching on this for solution but any insights will help surely.

Lottie File used(White version of this):
https://lottiefiles.com/9789-burger-menu

Lottie Code:

<Lottie
          animation={hamburgerMenuAnimation}
          options={{
            loop: false,
            autoplay: false,
          }}
          ariaLabel="Hamburger Menu"
/>

Video for problem:
https://user-images.githubusercontent.com/47962312/195103587-c5935543-3673-4f10-a6bc-c48be563200d.mp4

@mdubus
Copy link
Member

mdubus commented Oct 13, 2022

Hello @sushantgwr87 👋

Thanks a lot for reporting here your issues with the Lottie file. Indeed, I can see can the Lottie animation I provided in the issue goes from front to back and then back to front, and it's not ideal in our context.
I saw, like you, on Lottie's documentation that they have a segment option that could allow us to select a section to play, but just as you said, it is not yet implemented in the react-lottie library.

Therefore, I adapted the Lottie animation thanks to the Lottie Editor, and I just cropped the end of the animation. Here it is:
hamburger-menu.json.zip

(as GitHub doesn't support json files as attachments, I just compressed it as a zip. You'll have to unzip it 😉).

Then, you can play it from back to front regarding if the modal is opened or not, with the direction option: direction={dialog.visible ? 1 : -1}

Don't hesitate if you have any questions or if you need further help on this.

Have a nice day! ✨

@sushantgwr87
Copy link
Contributor Author

Hello Ma'am 😃,

Great Thanks for that Lottie animation file and guidance as well.

I updated the Lottie code in MobileHeader File:
Link to file code

<Lottie
          direction={dialog.visible ? 1 : -1}
          animation={hamburgerMenuAnimation}
          options={{
            loop: false,
            autoplay: false,
          }}
          ariaLabel="Hamburger Menu"
 />

I added your given Lottie json file in Lottie folder and imported them as needed.

Kindly check once if everything is correct or any other update/fix is needed.

Thanks for help 😸

src/blocks/Header/MobileHeader.js Outdated Show resolved Hide resolved
Copy link
Member

@mdubus mdubus left a comment

Choose a reason for hiding this comment

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

Almost perfect ✨

Can you please:

  • Check if the old Hamburger and Cross icon are still used, and delete them if it's not the case (check the svg icon & rerun the yarn icons command if necessary, also check the Icons story of Storybook) 🙏
  • Rebase your PR on main

Thanks a lot! 🦕

@sushantgwr87 sushantgwr87 reopened this Oct 13, 2022
@sushantgwr87
Copy link
Contributor Author

Hello,

Sorry for mess-up and for any inconvenience I caused during rebasing and updating files.

Actually I am still confused what should I do regarding rebasing my PR to main. I have never done this so I am clueless, If you can guide me then it will very helpful.

I have updated both main and this repo, after messing things up badly 🥲 . I used rebasing commands to update branches but still confused what actually I need to do here.

Other Details:

I have removed unused Icons from MobileHeader.js File and checked for any other icons. I have not deleted any icon file though.

Thanks and Sorry for asking too much help 😓

@mdubus
Copy link
Member

mdubus commented Oct 13, 2022

Hello @sushantgwr87

No problem 😀
I can see some conflicts in your PR that weren't quite well resolved and would erase some previous work.
Since you are not comfortable with git, I would suggest starting fresh and creating a brand new PR on a brand new branch 🚀

Here is how you can achieve it:

  • Go back to your main branch
  • Pull the changes that were made on the main branch from the Meilisearch repository
  • Create a new branch
  • Add the changes you want to the code
  • Then push the branch and open a new PR

Hope this helps ✨

@sushantgwr87
Copy link
Contributor Author

sushantgwr87 commented Oct 13, 2022

Thanks for guide Ma'am.

I updated my main repo and opened a new pull request.

You can check here PR.

@mdubus
Copy link
Member

mdubus commented Oct 13, 2022

Thanks @sushantgwr87 🙏

Closing in favor of #513

@mdubus mdubus closed this Oct 13, 2022
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.

Add animation to the burger menu
3 participants