search

devedmonton / DES-Website

The Dev Edmonton Society website! We empower Edmonton Developers!
https://devedmonton.com
MIT License
22 stars 52 forks source link

Hover issue is now solve #277

Closed LalitDeore closed 10 months ago

LalitDeore commented 10 months ago

Issue detail. After hovering to the "about our slack" button, it was disappearing. and i change the VButton component to the normal button and add own tailwind css to it.

Normal Button

normal

Hover Button

hover

MandyMeindersma commented 10 months ago

Okay so a couple things!

  1. I noticed you deleted the pr template when making this pr. It is here to help you and has a couple checklist items to help you. One of the checklist items is about running certain linting commands which will make sure that you haven't broken the build. So if you want to try making a new pr and not deleting the template I think that would be a good learning opportunity.
  2. You can see the the build failed on this pr. If you click "details" the the right of netlify/dev-edmonton/deploy-preview then you will see that it failed because of linting things. It tell you to run prettier. image There are two commands that will fix this that are listed in our readme. Read through that and see if you can find them, if you get stuck, let me know

Finally:

You changed VButton to be a regular button, that isn't ideal because that isn't what the issue was asking, we only want to change the hover state. So when you are approaching this problem first I would try to investigate anything that you don't know what it is. VButton is actually a component that we made. If you look around the code there are actually a couple instances of it: https://github.com/search?q=repo%3Adevedmonton%2FDES-Website%20vbutton&type=code Your changes only fix VButton in one spot, we want to make sure they are fixed in all the spots. Whats the best way to do that?

LalitDeore commented 10 months ago

Hey @MandyMeindersma First of all, thank you so much for having a detailed explanation of the problem in my PR.

  1. I am new to open source; that's why I got confused when I first saw the PR template. I will make sure to follow the PR template for the next issue.
  2. I didn't understand the second point: which command do I have to run, or what is it for? Could you please give me more details?
  3. I changed my previous code because it was updating only one button. Now keep the same component of VButton and and updated css for VButton. Please review these changes and tell me if is right or if needed different change.

Hover Button: hoverButton

MandyMeindersma commented 10 months ago

Yay for being new to open source! Glad you made a pr! So exciting!

just a couple more changes and then we should be good to merge!

As for the commands you have to run, you didn't break anything this build so you don't have to run them but it's good to know what all of the commands in the readme do. I was talking about these ones:

# run the linter
$ npm run lint-check # check for linting errors
$ npm run lint-fix # fix linting errors

# run the formatter
$ npm run format-check # check for formatting errors
$ npm run format # fix formatting errors
MandyMeindersma commented 10 months ago

Oh also one more thing. When you click the "Details" button now you can see a test website with your changes. When I hover I don't see the border you added. I think adding more hover: like I suggested should fix that

image

MandyMeindersma commented 10 months ago

Oh the joys of dealing with merge conflicts now too! You really are getting the full experience of open source. Let me know if you need any help with the merge conflicts

LalitDeore commented 10 months ago

Yes, It will be helpful if you tell me how I can solve the merge conflict issue.

MandyMeindersma commented 10 months ago

Try doing some googling or following this tutorial https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line

If you still have questions after that let me know :)

LalitDeore commented 10 months ago

dealing with merge conflicts

Is this issue resolved? I have solved the merge conflict with the main branch. Please checkout once.

MandyMeindersma commented 10 months ago

Almost! We are so close!

You just have one other suggestion left from me. If you go to the "files changed" tab it should show. I just need one more hover:

LalitDeore commented 10 months ago

Added one more hover

MandyMeindersma commented 10 months ago

Looks Great!

MandyMeindersma commented 10 months ago

@all-contributors add @LalitDeore for code

allcontributors[bot] commented 10 months ago

@MandyMeindersma

I've put up a pull request to add @LalitDeore! :tada: