Add Controlled state to useToggleLayer.

maxime4000 commented 4 years ago

Allow passing triggerRef when controlled. Issues #38 (Hack fix, I fix what I need and I could test, not fully resolve) Adjust type, Omit triggerRef in ToggleLayer due to not been tested. Fix openFromRef where ClientRect is not updated after scroll or resize. Issues #40

Question :

I want to talk about the open, toggle and close function in RenderChildrenProps in ToggleLayer. I understand that if your controlling the layer from outside, you shouldn't be able to do those which is understandable, but because it throw error if isOpenExternal is set, it force dev to protect their function if they allow to controlled the state. This is the useToggleLayer version, but it apply to ToggleLayer too.

const protectedClose = () => {
  if(!isShown){
    toggleLayerProps.close()
    handleFocus()
  }
}
const protectedToggle = () => {
  if (!isShown && !toggleLayerProps.isOpen) {
    toggleLayerProps.openFromRef(targetRef)
  } else {
    protectedClose()
  }
}

I kinda think this error handle in ToggleLayer do more bad than good :

if (isSet(isOpenExternal)) {
  throw new Error(
    "You cannot call `open()|close()|toggle()` while using the `isOpen` prop"
  );
}

If I want to controlled, I could said that my isOpenExternal is my "Always on top" feature and if it's disable, I can show it like a normal ToggleLayer, but with this error, you cannot... That been said, I didn't add the error in useToggleLayer in the open function now that I allow it to be controlled, because of this reason.

Note :

I didn't include the triggerRef props in ToggleLayer because I had no code that could test if it work. Here what I would have add, but because I couldn't test it, I omit it and the type triggerRef in ToggleLayerProps.

React.useEffect(() => {
  triggerRefExternal = triggerRef
}, [triggerRef]);

Why I only change useToggleLayer : I'm developing some Layout Primitive component. Something similar to Evergreen Pane and I'm restricted to a styling library which is something I shouldn't change, because it can affect much more than what I want to do. So I'm trying to omit the more element I can because the CSS has a lot of > selector and adding a Tooltip element to hold the ref break CSS. So I try to forwardRef and ToggleLayer give me this error Error: Could not find a valid reference of the trigger element. See https://www.react-laag.com/docs/togglelayer/#children for more info. or this one when it wasn't ToggleLayer : https://reactjs.org/warnings/special-props.html I convert my component to useToggleLayer and had better result just because I can control the ref outside of the ToggleLayer instead of having to ref={mergeRef([triggerRef, externalRef])} So here is why my changes only apply to useToggleLayer.

Also, I'm sorry I didn't add docs to explain those new options. I didn't know where to add them... ToggleLayer explain props but useToggleLayer has one props that ToggleLayer doesn't.

Feel free to comment or edits.

everweij commented 3 years ago

Hi @maxime4000, First of all my apologies for this late response. I've been really busy lately with my job and had some pressure to work on v2.0 of this library. I do really appreciate your time and effort!

Maybe you've noticed, but I released v2 yesterday, which will probably impact your issue / PR a lot. The most important differences in v2 regarding this PR are:

only the hook is left, creating possibilities to better compose higher-level solutions -> all the 'magic' happening is a lot more transparent because you have full control over the props and how you structure your elements
Everything is now controlled by default. I think this will make the flow easier to understand, and besides, adding 1 extra line (React.useState()) will not hurt that much.

What do you think? Does v2 better integrate with the stuff you're working on?

Thanks, Erik

maxime4000 commented 3 years ago

I will take time in the next days to look at it and I'll let you know after that. My main concern was that some feature in the uncontrolled version wasn't there in the controlled version, and I couldnt get my ref working in the uncontrolled version but could make it work in the controlled one. I'll write back in the next days.

everweij commented 3 years ago

Thanks @maxime4000!

maxime4000 commented 3 years ago

I'm going to close this PR because it's not useful anymore. I'm sorry, I've been busy and I couldn't come back to the component I was writing with V1. It seem like what I needed has it's alternative now, but I cannot confirm if it work in the context I was using it. If I ever comeback to this and it does not work, I will write an issue. But for now, I will close this PR.

everweij / react-laag

Add Controlled state to useToggleLayer. #49

Question :

Note :