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

Feat: Improve Dropdown Component #2492

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

ArthurTriis1
Copy link
Contributor

@ArthurTriis1 ArthurTriis1 commented Oct 4, 2024

What's the purpose of this pull request?

Improve the Dropdown by adding new options and expanding its usability. In this PR, properties like asChild, alignment, and dismissOnClick have been added. There are no breaking changes, but properties related to icons in DropdownButton and DropdownMenuItem were marked as deprecated, as these functionalities can now be configured directly within the component content.

How does it work?

New Hooks

As part of the refactor of the DropdownButton and DropdownMenuItem components, new hooks were created that encapsulate logic, enabling new components to be created with a fresh structure while maintaining functionality. The new hooks are useDropdownItem and useDropdownTrigger.

Alignment Property

A new property called align has been added to the DropdownMenu. This property allows the DropdownMenu to align with the DropdownButton based on the desired alignment: right, left, or center.

right center left
Screenshot 2024-10-07 at 23 24 07 Screenshot 2024-10-07 at 23 23 31 Screenshot 2024-10-07 at 23 24 35

Adjustments to the useDropdownPosition Hook

Some adjustments have been made to the useDropdownPosition hook. It now observes changes in window resizing, fixing the alignment bug for the Dropdown.

Controlled Component

New functionality has been added to allow the Dropdown to be controlled externally, enabling it to be manipulated through external states. Example:

      const [isOpen, setIsOpen] = useState(false)
      return (
        <Dropdown isOpen={isOpen} onDismiss={() => {setIsOpen(false)}}>
          <DropdownButton>Controlled Dropdown</DropdownButton>
          <DropdownMenu>
            <DropdownItem>A - Dropdown item as child</DropdownItem>
            <DropdownItem>B - Dropdown item as child</DropdownItem>
            <DropdownItem>C - Dropdown item as child</DropdownItem>
            <DropdownItem>D - Dropdown item as child</DropdownItem>
          </DropdownMenu>
        </Dropdown>

The usage of the controlled Dropdown can be seen in the preview at: #controlled

asChild Property

Based on libraries like Radix, the DropdownButton and DropdownMenuItem components now include the asChild property. This allows replacing the default rendered components with custom child elements, while maintaining behavior through prop passing. Example:

      <Dropdown>
        <DropdownButton asChild>
          <button>{'Dropdown with trigger as child'}</button>
        </DropdownButton>
        <DropdownMenu style={{ backgroundColor: "white"}}>
          <DropdownItem asChild>
            <button>A - Dropdown item as child</button>
          </DropdownItem>
          <DropdownItem asChild>
            <button>B - Dropdown item as child</button>
          </DropdownItem>
          <DropdownItem asChild>
            <button>C - Dropdown item as child</button>
          </DropdownItem>
          <DropdownItem asChild>
            <button>D - Dropdown item as child</button>
          </DropdownItem>
        </DropdownMenu>
      </Dropdown>

The usage of the asChild property can be seen in the preview at: #as-child

How to test it?
You can test it by visiting the documentation at: https://faststore-site-git-feat-improve-dropdowneat-faststore.vercel.app/components/molecules/dropdown

References
Dropdown Menu
B2BTEAM-1895

Checklist

You may erase this after checking them all 😉

PR Title and Commit Messages

  • PR title and commit messages follow the Conventional Commits specification
    • Available prefixes: feat, fix, chore, docs, style, refactor and test

PR Description

  • Added a label according to the PR goal - breaking change, bug, contributing, performance, documentation..

Dependencies

  • Committed the yarn.lock file when there were changes to the packages

Copy link

vercel bot commented Oct 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
faststore-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2024 0:55am

Comment on lines +4 to +19
// const { pathname } = new URL(request.url)
// if (pathname.startsWith('/docs')) {
// return NextResponse.redirect(
// `https://developers.vtex.com/docs/guides/faststore${pathname.replace(
// '/docs',
// ''
// )}`
// )
// }
// if (pathname.startsWith('/components')) {
// return NextResponse.redirect(
// `https://developers.vtex.com/docs/guides/faststore${pathname
// .replace('/components', '')
// .replace(/\/([^\/]*)\//, '/$1-')}`
// )
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uncomment before the merge

Comment on lines 235 to 260
#### Controlled

export const ControlledDropdown = () => {
const [isOpen, setIsOpen] = useState(false)
return (
<Dropdown isOpen={isOpen} onDismiss={() => {console.log("Dismiss"); setIsOpen(false)}}>
<DropdownButton asChild>
<button onClick={() => setIsOpen(old => !old)}>{'Controlled Dropdown with trigger as child'}</button>
</DropdownButton>
<DropdownMenu style={{ backgroundColor: "white"}}>
<DropdownItem asChild>
<button>A - Dropdown item as child</button>
</DropdownItem>
<DropdownItem asChild>
<button>B - Dropdown item as child</button>
</DropdownItem>
<DropdownItem asChild>
<button>C - Dropdown item as child</button>
</DropdownItem>
<DropdownItem asChild>
<button>D - Dropdown item as child</button>
</DropdownItem>
</DropdownMenu>
</Dropdown>
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds controlled option for Dropdown Component

Comment on lines +101 to +118
const handleKeyNavigatePress = (key: string) => {
const dropdownItems = dropdownItemsRef?.current ?? [];
const selectedIndex = selectedDropdownItemIndexRef!.current;

const rearrangedDropdownItems = [
...dropdownItems.slice(selectedIndex + 1),
...dropdownItems.slice(0, selectedIndex + 1),
];

const matchItem = rearrangedDropdownItems.find(
(item) => item.textContent?.[0]?.toLowerCase() === key.toLowerCase()
);

if (matchItem) {
selectedDropdownItemIndexRef!.current = dropdownItems.indexOf(matchItem);
matchItem.focus();
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add key navigation to Dropdown component

Copy link

codesandbox-ci bot commented Oct 4, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

dismissOnClick?: boolean
}

export const useDropdownItem = <
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extract the logic of DropdownItem to a reusable hook

Comment on lines 21 to 44
useEffect(() => {
const updateMenuPosition = () => {
// Necessary to use this component in SSR
const isBrowser = typeof window !== 'undefined'

const buttonRect = dropdownTriggerRef?.current?.getBoundingClientRect()
const topLevel = buttonRect?.top ?? 0
const topOffset = buttonRect?.height ?? 0
const leftLevel = buttonRect?.left ?? 0

// The scroll properties fix the position of DropdownMenu when the scroll is activated.
const scrollTop = isBrowser ? document?.documentElement?.scrollTop : 0
const scrollLeft = isBrowser ? document?.documentElement?.scrollLeft : 0

const topPosition = topLevel + topOffset + scrollTop

const leftPosition = leftLevel + scrollLeft

setPositionProps({
top: topPosition,
left: leftPosition,
loading: false
})
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update Dropdown position based on window resize

Comment on lines +9 to +26
export const useDropdownTrigger = <T extends HTMLElement = HTMLElement>({
triggerRef,
}: UseDropdownTriggerProps<T>) => {
const { toggle, dropdownTriggerRef, addDropdownTriggerRef, isOpen, id } =
useDropdown<T>()

useImperativeHandle(triggerRef, () => dropdownTriggerRef!.current!, [
dropdownTriggerRef,
])

return {
onClick: toggle,
ref: addDropdownTriggerRef,
'aria-expanded': isOpen,
'aria-controls': id,
'aria-haspopup': 'menu' as const,
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extract the logic of DropdownButton to a reusable hook

@ArthurTriis1 ArthurTriis1 self-assigned this Oct 4, 2024
@ArthurTriis1 ArthurTriis1 added the contributing Pull request submitted by the community label Oct 4, 2024
Copy link
Contributor

@hellofanny hellofanny left a comment

Choose a reason for hiding this comment

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

Nice work! I think it's working as expected.
We just need to make sure to update our docs related to this component :)

* Specifies the size variant.
*/
size?: 'small' | 'regular'
/**
* Alignment for the dropdown
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Alignment for the dropdown
* Specifies the dropdown alignment

@renatomaurovtex renatomaurovtex dismissed lariciamota’s stale review October 29, 2024 13:11

Foi feita a alteração, como a Lari esta de ferias estou fazendo o dismiss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributing Pull request submitted by the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants