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

feat: consolidate "help" elements into help menu #1118

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

Conversation

vicksey
Copy link
Contributor

@vicksey vicksey commented Jan 15, 2025

Summary

Aggregated the "help" elements, Help Box, Feedback Form, and Tutorial, into one help menu button. Once hovered the three elements are shown and can be clicked to be used.

Test Plan

  • Hover over the Help Menu to ensure all necessary components appear and can be accessed
  • Click on the Help Menu to ensure children components appear/disappear
  • Click through each element to test that they work like they normally do elsewhere
  • Restart Tutorial button works
  • Feedback Form opens in new tab
  • Help menu appears
  • See if size and design is good

Issues

Closes #1053

@vicksey vicksey requested a review from adcockdalton January 15, 2025 00:49
@vicksey vicksey self-assigned this Jan 15, 2025
@vicksey vicksey changed the title Fixed button functionality feat: consolidate "help" elements into help menu Jan 15, 2025
Copy link
Collaborator

@adcockdalton adcockdalton left a comment

Choose a reason for hiding this comment

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

Good work so far, but there are still a couple of bugs I'm noticing:

  1. Help box does not re-open every time icon is clicked. Steps to repro:
  • Click on help box icon
  • X it out
  • Click on help box icon again. It will not show, since clicking it just attempts to close the already-closed help box.
  1. Mouse-off delay is slightly too long. I like the feature (great suggestion by @KevinWu098 ) but a collapse timeout of 1 second is quite long. Try out some smaller times and see how it feels. Maybe half a second?

  2. Mouse-off delay only works from the blue circle. If there is no way to implement this natively with mui then we can settle with a short mouse-off delay and not address this, but it would be apt for the delay to also work from the icon buttons (right now it doesn't).

  3. The tutorial dies if you mouse over the help menu while the tutorial is running.

  • Click restart tutorial
  • Mouse-off the menu
  • Mouse back onto the menu once it closes

If you need help with implementation or have questions about code, let us know in the Discord ASAP

LGTM: light mode, help box design

Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

Just throwing up some comments to work on

@vicksey
Copy link
Contributor Author

vicksey commented Feb 1, 2025

@adcockdalton I've fixed everything and cleaned up my code it should be ready for a final review now. Let me know if anything comes up!

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.

Using event emitters and listeners in RightPaneStore is a broken window, but I'll let it slide because we'll be refactoring this.

@MinhxNguyen7
Copy link
Member

@KevinWu098 @adcockdalton Can you check if your comments have been addressed? This has been sitting for a while

@KevinWu098
Copy link
Member

@KevinWu098 @adcockdalton Can you check if your comments have been addressed? This has been sitting for a while

mm, I had a comment about refactoring logic into a Zustand store specifically for the help elements, but that can be in a follow up -- don't think I have anything blocking now

@adcockdalton
Copy link
Collaborator

@KevinWu098 @adcockdalton Can you check if your comments have been addressed? This has been sitting for a while

@vicksey
I only have nitpicks at this point. The changes look great.

  1. It doesn't really make sense from a design standpoint to have the help button ripple on click if clicking it has no effect. If this is removable that would be advisable.
  2. The best behavior/appearance of the help button during the tutorial is sort of a gray area. Right now, it just sits there and does not open on hover. I'll leave it up to you what behavior you think it should have (I'm good to approve as-is, but I want to make sure it's intentional).

Nice PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review 🧐
Development

Successfully merging this pull request may close these issues.

feat: consolidate "help" elements into help menu
4 participants