[Component] Dropdown

[amnambiar]

Pull request template

Description

Details of this Dropdown component and its behaviour is as per this document

Task

• https://github.com/input-output-hk/high-assurance-private/issues/285

Checklist

• Commit messages broadly make sense and follow the Conventional Commits format.
• PR targets master/develop
• New tests are added for the changes (if not, mention it in the PR description). These may include:
  • Unit tests,
  • Integration tests,
  • Property-Based testing,
  • End-to-end tests.
• The new version builds and passes all tests. If not, please mention it in the PR description.
• Self review of the code has been done.
• Reviewer has been requested.

[KJES4]

I've left some comments requesting changes to a few files.

[KJES4]

This line of code is canceling out the position absolute styles you applied to the div that is also receiving the floatingStyles from the style prop. This strategy: "fixed" line of code can be removed.

[amnambiar]

Good catch! 'absolute' is unnecessary when using floating-ui.

But, I would like to decide if we want to proceed floating-ui before i correct these areas.

[amnambiar]

Couldn't find the popovers to be aligned properly without this style for floating. I'd suggest we retain the floating-ui in Dropdown for the moment, and rework on it (remove or handle the popup aligning manually) after the main components are ready. I will not use the floating-ui in the Selectbox

[KJES4]

If that is the case there is something incorrect with the styles in the file you are working on and we need to have a call to figure out what the issue is. There is no reason to keep this package when the problem stated for why it is needed can be solved by adding a single tailwind class name.

[KJES4]

Since the div doesn't have any margin or padding and the button doesn't have a margin, you can remove the element and attach the ref to the root container div element.

[amnambiar]

While using floating-ui, ref has to be attached to the element respective to which the floating popover should be placed. hence here.

[KJES4]

Comment resolved, floating-ui was removed.

[KJES4]

dropdownRef isn't being assigned to any element so it isn't actually being used for anything.

[amnambiar]

That was a miss.

Should have been

return (
    <div className="relative" ref={dropdownRef}>
      <span ref={refs.setReference}>

But, that is unnecessary when using floating-ui as it itself handles close-on-click-outside, so we wouldn't need to attach a separate 'click' event listener to close the popup when clicked outside of target area.

Let me know what your ideas are on keeping the floating-ui package intact. And, i can do this correction.

[amnambiar]

Commit error. Supposed to be attached here-

return (
    <div className="relative" ref={dropdownRef}>
      <span ref={refs.setReference}>
        <Button

[KJES4]

Comment resolved, floating-ui was removed.

[KJES4]

On my local instance I removed all of the floatingUI code and I added the ref to the root parent div and all the functionality seems to be working as expected. Remind me again, what was the rational for add this third party library?

[amnambiar]

Consider the scenario of our Sort, Filter buttons above the Counterexample Trace table. They are aligned to the far left of the container. In that scenario, we need to determine dynamically how our dropdown list popup should be opened so that it is positioned correctly. When no particular styles applied, it will be placed like in below screengrab -

Even with just flex-direction: row-reverse for a container that holds the buttons, and position:absolute for the popover div, we still will need to dynamically determine within the DropdownComponent if there is enough viewport space for the dropdown contents to be visible, then set right:0 / left:0 respectively.

We would have to deal this entirely with our set of calculations, but with floating-ui it is handled in much cleaner/easier with minimal effort from our end.
floating-ui.webm

let me know if we do not want to seek support from any third-party packages big/small, and I can edit to handle this vanilla.

[KJES4]

Keep all the style changes I told you to make, don't change anything else, but add right-0 to the popup div element that has the absolute position and let me know how that works for you. This worked for me.

[amnambiar]

We shall discuss this in person.

[amnambiar]

Discussed - Use a position prop left/right for now and style with class accordingly.

[KJES4]

There could be a case where a user opens the select menu options, choses an option to see what it does and then wants to click on a different option. If the setOpen to false is here that would mean the user has to re-open the menu each time they want to select a new radio button option. I think the menu button close event should work the same regardless of the content shown inside of it.

[amnambiar]

Agree, done 👍

[KJES4]

Since you are trying to setState from inside of a useEffect hook this state is considered a reactive value. The problem with an empty dependency array is that you will end up with a state value for the open state. You could include 'open' in the dependency array but then this would cause the eventListener to be torn down and setup each time the value for open changes, which isn't ideal. You can update the code as follows that way you can ensure you aren't falsely calling the setOpen(false) statement.

const openRef = useRef(open);

  useEffect(() => {
    openRef.current = open;
  }, [open]);

  useEffect(() => {
    const listener = (event: MouseEvent) => {
        if (!openRef.current || dropdownRef.current?.contains(event.target as Node)) {
        return;
        }
        setOpen(false);
    };

    document.addEventListener("click", listener, true);
    return () => {
       document.removeEventListener("click", listener, true);
    };
  }, []); 

[amnambiar]

Will look into this after deciding upon whether or not to keep floating-ui.

[amnambiar]

[] in useEffect, attaches a single global listener and is ideal for as long the useEffect does not reply on a stale state open (in this case which isn't)

The suggestion to check openRef.current if dropdown popover is currently open, is 💯 right, and thus added that in.

I'm commiting my change for this bit .

[KJES4]

There are two useEffects above, which one are you talking about in your first sentence?

[amnambiar]

The second one which binds listener to setOpen(false) when clicked anywhere outside the dropdown targets.

Note: floating-ui offers useDismiss to handle this scenario. Then we would not have to bind additional events to listen the same, given we are continuing with the package.

We shall discuss this as well in a call

[KJES4]

Comment resolved, floating-ui was removed.

[KJES4]

There are two label props defined in this component, let's made the code easier to read by identifying that the labels correspond to. You can call this label btnLabel and the one on the options interface itemLabel.

[amnambiar]

btnLabel is updated. Retained label within options , as if it is changed to itemLabel would have to change value to itemValue for naming consistency. Hence,

[KJES4]

If that is the case there is something incorrect with the styles in the file you are working on and we need to have a call to figure out what the issue is. There is no reason to keep this package when the problem stated for why it is needed can be solved by adding a single tailwind class name.

[amnambiar]

@KJES4 ,

I've pulled your latest changes. But I've updated the way defaultChecked option was passed into the Dropdown component. Do have a look. And I suggest, removing the file DropdownMenu.tsx

[KJES4]

Since floating-ui is no longer being used please make sure it is uninstalled and removed from the package.json file.

[KJES4]

@KJES4 ,

I've pulled your latest changes. But I've updated the way defaultChecked option was passed into the Dropdown component. Do have a look. And I suggest, removing the file DropdownMenu.tsx

@amnambiar are you saying on your end there are two files Dropdown.tsx and DropdownMenu.tsx and that we should remove the latter one? If so that is fine. We can keep the shorter name and keep the first file.

[KJES4]

Can you move the Dropdown components into the third column so they don't break the layout on this page and cause a scroll bar to appear?

[KJES4]

What is the rational for using useCallback here?

[KJES4]

In the dropdownMenu component I added a size property. Can you take a look at how I implemented that and add that prop to this file? This will help allow the component to be more condensed when used inside of a table.