Skip to content

The "+" button that toggles the menu with options to create a folder and create a file does not close the menu when clicking anywhere else on the screen. #2524

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

Closed
gauravsingh94 opened this issue Oct 21, 2023 · 6 comments
Labels
Enhancement Improvement to an existing feature

Comments

@gauravsingh94
Copy link
Contributor

gauravsingh94 commented Oct 21, 2023

Increasing Access

When we are clicking on the plus "+" for creating the folder or file the menu which get toggle is not close when we click any where on the screen.

Feature enhancement details

This will increase the user experience.

Screencast.from.2023-10-21.19-20-49.webm
@gauravsingh94 gauravsingh94 added the Enhancement Improvement to an existing feature label Oct 21, 2023
@welcome
Copy link

welcome bot commented Oct 21, 2023

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

gauravsingh94 added a commit to gauravsingh94/p5.js-web-editor that referenced this issue Oct 21, 2023
@gauravsingh94
Copy link
Contributor Author

I have fix this issue and i have raise the pr.

Final Output will look like this.

Screencast.from.2023-10-21.20-11-46.webm

gauravsingh94 added a commit to gauravsingh94/p5.js-web-editor that referenced this issue Oct 22, 2023
@lindapaiste
Copy link
Collaborator

lindapaiste commented Dec 10, 2023

I did some investigating here and the core issue is a stale closure of the isFocused variable in the onBlurComponent handler.

const onBlurComponent = () => {
setIsFocused(false);
setTimeout(() => {
if (!isFocused) {
dispatch(closeProjectOptions());
}
}, 200);
};

The value of isFocused which is evaluated inside of the setTimeout will be the value at the time when the onBlurComponent handler is executed, not the value after the 200ms delay. React handles the setIsFocused(false) state update asynchronously so we are getting the value before that update, when it was true, and therefore the line dispatch(closeProjectOptions()); is never executed because if (!isFocused) is always false.

This setup wasn't/isn't an issue in the class components because we're looking at a property on an object, this.state.isFocused, so there is no stale variable. For the record, I'm the one who converted this to a function component so this bug is my fault as I didn't realize the mistake at the time.

I handled it properly in a new DropdownMenu component where I'm using a ref instead of a state to avoid the stale closure issue. When using a ref we see the value of focusedRef.current after the 200ms delay.

// Note: need to use a ref instead of a state to avoid stale closures.
const focusedRef = useRef(false);

const handleBlur = () => {
focusedRef.current = false;
setTimeout(() => {
if (!focusedRef.current) {
close();
}
}, 200);
};

IMO the best course of action for the long term would be to use that DropdownMenu component here and remove the buggy logic which is specific to this component. But there are two holdups there:

  1. Need to override the sizing on the dropdown so that it shrinks to fit like the current one.
  2. Need to figure out how to control the state externally in order to support opening the dropdown when right-clicking the header.

@Ri-Sharma
Copy link
Contributor

@lindapaiste in the below part of code we don't actually need setTimeout function

const onBlurComponent = () => { 
   setIsFocused(false); 
   setTimeout(() => { 
     if (!isFocused) { 
       dispatch(closeProjectOptions()); 
     } 
   }, 200); 
 }; 

It will also work if we just dispatch without the setTimeout function. Correct me if I am wrong!

const onBlurComponent = () => { 
   setIsFocused(false); 
   dispatch(closeProjectOptions());
 }; 

@lindapaiste
Copy link
Collaborator

@lindapaiste in the below part of code we don't actually need setTimeout function

const onBlurComponent = () => { 
   setIsFocused(false); 
   setTimeout(() => { 
     if (!isFocused) { 
       dispatch(closeProjectOptions()); 
     } 
   }, 200); 
 }; 

It will also work if we just dispatch without the setTimeout function. Correct me if I am wrong!

const onBlurComponent = () => { 
   setIsFocused(false); 
   dispatch(closeProjectOptions());
 }; 

Hey so I think that the intention of the setTimeout is that we close the dropdown when one item is blurred and nothing else is focused in its place. It makes more sense if you thing about using the Tab key to navigate instead of mouse clicks. In the class component version we have the onBlurComponent handler on each of the items in the menu. The Tab key goes through each item of the menu, blurring one item and focusing on the next. We fire the "close" handler when you tab off of the last item in the list.

When the focus changes from the first item in the list to the second, it goes through these steps:

  1. onBlur is called on the first item.
  2. isFocused is set to false.
  3. The timeout begins.
  4. onFocus is called on the second item.
  5. isFocused is set to true.
  6. 200 ms elapses.
  7. The timeout is executed.
  8. No "close" action is dispatched because isFocused is true.

In the DropdownMenu function component I lifted the onBlur handling up to to the list component rather than the individual list items, so the timeout checking is less necessary. But we still have a behavior that keeps the menu open if you Tab-Shift backwards onto the dropdown arrow.

@Ri-Sharma
Copy link
Contributor

Ri-Sharma commented Jan 3, 2024

@lindapaiste How about integrating an external library for this functionality? That way we don't have to apply this on our own and we can just add extra functions on top of that.

I was looking around and found This React Component, you can check it. We can also add it to all dropdowns in the web app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvement to an existing feature
Projects
None yet
4 participants