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: UX Refactor - 3 (#42) and SearchBox Optimization(#53) #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pawansoni007
Copy link

UX Refactor (#42):

  • Add placeholder in search box
  • Add bottom padding equal to top navbar for balanced UI spacing
  • Show all command entries when the search box is in empty/initial state.

SearchBox Optimization (IMPORTANT): #53

  • Fix: SearchBox re-rendering every second due to usePlayground custom hook's timer
  • Memoize SearchBox to prevent unnecessary updates
  • Decouple from custom hook's timer updates

UX Refactor (DiceDB#42):
- Add placeholder in search box
- Add bottom padding equal to top navbar for balanced UI spacing
- Show all command entries when the search box is in empty/initial state.

SearchBox Optimization (IMPORTANT):
- Fix: SearchBox re-rendering every second due to usePlayground custom hook's timer
- Memoize SearchBox to prevent unnecessary updates
- Decouple from custom hook's timer updates
@lucifercr07
Copy link
Contributor

@pawansoni007 please attach screenshots for the changes, and fix the build.

@pawansoni007
Copy link
Author

pawansoni007 commented Oct 6, 2024

I'll do the needful for the prettier and push the updated commit. Thanks

Btw..what screenshots are you seeking for?

@KaviiSuri
Copy link
Contributor

I'll do the needful for the prettier and push the updated commit. Thanks

Btw..what screenshots are you seeking for?

@pawansoni007 , screenshots of the changes you made help us review PRs faster, otherwise it's hard to understand what changed without. testing locally.

Copy link
Contributor

@KaviiSuri KaviiSuri left a comment

Choose a reason for hiding this comment

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

Thank you so much for contributing!

I also had a few suggestions on the UI changes: https://www.loom.com/share/8c34be797b4740ea85a9f8da27e3e603?sid=97c0970e-df58-4999-a110-8e6ec40f1cd7

usePlayground();

const MemoizedSearchBox = useMemo(() => <SearchBox />, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a hack we had to do because we don't have separate components for the 2 sections of playground (The Terminal and the Searchbox).

Can we just create a component for the Left Column of Playground (Terminal + Commands Left etc), and call usePlayground from it? Basically, neither the Searchbox or any of it's parents should care about the time or commandsLeft, these should be state of the sibling for Searchbox.

@KaviiSuri
Copy link
Contributor

@pawansoni007 could you please address the conflicts?

@pawansoni007
Copy link
Author

@pawansoni007 could you please address the conflicts?

Brother, is it okay if i do this on Friday evening, I'm mysrlf feeling bad 'bout keeping it this long, something came up and I got completely engaged into that.

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.

3 participants