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.

Sign up for GitHub

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

Jump to bottom

feat: changing pop-over x button for hover :) and adding padding to the popover to make it more visible #1012

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: changing pop-over x button for hover :) and adding padding to the popover to make it more visible #1012

wants to merge 3 commits into from

Conversation

Priyansh4444
Copy link
Member

Summary

Making the UX just a bit better by creating a transition for the hover on the button

Test Plan

Running the frontend

Issues

  • Currently the 4th item on the path goes a bit too left on the screen
  • Some steps also go a bit off the page

Closes #1011

@github-actions github-actions bot requested a review from MinhxNguyen7 October 16, 2024 20:43
MinhxNguyen7
MinhxNguyen7 requested changes Oct 17, 2024
View reviewed changes
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

Code-wise, the button should not be defined with the router; that just doesn't make logical sense. it should go in components/buttons. Also, since we drink the MUI Kool-Aid, we shouldn't have to implement such a simple button. Take a look at this. We should be able to just define the button in-line with some customization. Do say though, if that is not fit-for-purpose.

I have a few things to ask in terms of documentation. These are somewhat matters of pedantry, but I think it's important to maintain standards.

Could you please give more information on the test plan? Yes, we run the front end, but what should we expect to see? You also say that there are some problems. Was that introduced by this PR, or was it there before? If the former is the case, why can't it be addressed? If the latter is the case, you should put that in a "Future Follow-Up" section and add it as an issue.

Also, the "Issues" section is for related GitHub issues. The issues that you have there should go in the summary.

@Priyansh4444
Copy link
Member Author

Thank you for the feedback I will keep this in mind going on when I contribute to other repositories as well!
I think it is fit to make its own component for the button.
There are two ways to edit the button, either by adding it as a component in styles or making a react functional component for it.

I think the MUI button is the best way to approach the matter since the onHover Effect then can be achieved!
though another solution could potentially be manaully taking the .react__tour_close class and adding it to App.css

I am still a bit unsure, how to test frontend other than visually, but in terms of expected result having the tourguide is partially going out of bounds of the screen and therefore cannot be seen I am unsure how to control where the tourguide goes.
The problems I mentioned happens before the PR as well. I will try and fix these issues when I get a bit more time since currently I am a bit busy!

Future Follow Up:

  • Fixing the TourGuide So it does not go out of bounds
  • Making the 4th step of the tour guide happen to the right of the calendar instead of left (it was on the right before the PR)

@github-actions github-actions bot added Stale and removed Stale labels Nov 16, 2024
@Priyansh4444
Copy link
Member Author

I have no idea how to test the front-end other than visually, and visually speaking I think its working and does a good job formatting the X button, any thoughts and guidance would be appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MinhxNguyen7 MinhxNguyen7 MinhxNguyen7 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Making the X button on Reactour bigger with padding and better UX?
2 participants
@Priyansh4444 @MinhxNguyen7