Skip to content

Conversation

britt6612
Copy link
Collaborator

@britt6612 britt6612 commented Aug 21, 2024

What does this PR do?

This PR checks to see if the user clicks on the target and then will not invoke the onClickOutside func

Where should the reviewer start?

dropContainer

What testing has been done on this PR?

Added a test

local story

import React, { useRef, useState } from 'react';
import { Box, Button, Drop, DropButton, Layer, TextInput } from 'grommet';

const alignRight = { left: 'right' };
const alignLeft = { right: 'left' };

const MultipleDrop = () => {
  const [showDrop, setShowDrop] = useState(false);
  const targetRef = useRef();

  return (
    // Uncomment <Grommet> lines when using outside of storybook
    // <Grommet theme={...}>
    <Box gap="medium" fill align="center" justify="center">
      <Button
        ref={targetRef}
        label="button"
        onClick={() => setShowDrop(!showDrop)}
      />
      {showDrop && (
        <Drop
          align={alignRight}
          target={targetRef.current}
          onClickOutside={() => setShowDrop(false)}
        >
          <Box pad="large">
            <TextInput
              value=""
              onChange={() => {}}
              suggestions={['one', 'two']}
            />
          </Box>
        </Drop>
      )}
    </Box>
    // </Grommet>
  );
};

export const Multiple = () => <MultipleDrop />;
Multiple.parameters = {
  chromatic: { disable: true },
};
Multiple.args = {
  full: true,
};

export default {
  title: 'Controls/Drop/Multiple',
};

How should this be manually tested?

use the above code to test locally

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

When you click on the target in which opens the drop if you have it set in your local state to close the drop this was not working properly when onClickOutside was present
see codesandbox
https://codesandbox.io/p/sandbox/jovial-mopsa-4ry469?file=%2Fsrc%2Findex.js%3A28%2C1-28%2C52

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

no

Should this PR be mentioned in the release notes?

yes

Is this change backwards compatible or is it a breaking change?

backwards compatible

@britt6612 britt6612 self-assigned this Aug 21, 2024
@britt6612 britt6612 added the bug issue that does not match design or documentation and requires code changes to address label Aug 21, 2024
@britt6612 britt6612 requested review from jcfilben and taysea August 21, 2024 18:31
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

I tested with the code you have in the PR description but I was still seeing the issue:

Aug-21-2024.14-09-21.mp4

@britt6612
Copy link
Collaborator Author

I tested with the code you have in the PR description but I was still seeing the issue:

Aug-21-2024.14-09-21.mp4

updated sorry forgot to also check for dropTarget.contains

@britt6612 britt6612 requested a review from jcfilben August 21, 2024 21:18
britt6612 and others added 2 commits August 21, 2024 20:20
Co-authored-by: Jessica Filben <54560994+jcfilben@users.noreply.github.com>
Co-authored-by: Jessica Filben <54560994+jcfilben@users.noreply.github.com>
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Looks good!

@jcfilben jcfilben merged commit c002e51 into grommet:master Aug 23, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issue that does not match design or documentation and requires code changes to address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants