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

Fix issue #67: Implemented scrollbar and toggle functionality for theme list #68

Merged
merged 2 commits into from
May 20, 2024

Conversation

imsachin49
Copy link
Contributor

Changes Made:

  • Added scrollbar to the theme list for easier navigation!
  • Implemented toggle functionality for the theme button to show/hide the theme list.

Details:

  • Added CSS styles to create a scrollbar for the theme list container
  • Modified the ThemeButton component to toggle the visibility of the theme list on clicking the theme button by utilizing the useState hook in React.
  • Tested the changes locally to ensure smooth functionality

Related Issue:

This pull request addresses issue #67

@baoliay2008
Copy link
Owner

@imsachin49 Thank you so much for your prompt pull request, excellent work.


Regarding the toggling functionality, I noticed three issues with its behavior:

  1. Click the theme button, click outside, then click the theme button again.
    67-bug-1.webm
  2. Click the theme button, select an item from the theme list, then click the theme button again.
    67-bug-2.webm
  3. Click the theme button, drag the scrollbar, then click the theme button again.
    67-bug-3.webm

To address these issues:

  • For Problem 1: We should add an event listener to detect when a click occurs outside of the dropdown component.
    67-bug-1-fixed.webm
  • For Problems 2 and 3: Simply remove the onBlur event.
    67-bug-2-fixed.webm
    67-bug-3-fixed.webm

Any other feedback is welcome. If you agree with these suggestions, I'll proceed with merging the pull request.

@imsachin49
Copy link
Contributor Author

Yeah those changes makes sense and i believe they effectively resolves the identified issues.
You can proceed with merging the pull request.

@baoliay2008 baoliay2008 merged commit 246f397 into baoliay2008:main May 20, 2024
2 checks passed
baoliay2008 pushed a commit that referenced this pull request Sep 17, 2024
…heme list (pull #68)

* Fix issue #67: added scroller to theme list

* 🐛 fix: Add event listener to close dropdown when clicking outside; remove onBlur event.

---------

Co-authored-by: L. Bao
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.

2 participants