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(popover): dynamic height management #1772

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

Powerplex
Copy link
Contributor

@Powerplex Powerplex commented Dec 19, 2023

TYPE(SCOPE): TITLE

TASK: #1771

Dynamic height resizing on Popover.

It allows Dropdown.Popover to optimize height dynamically and display as many items as possible.

Types of changes

  • ✨ New feature (non-breaking change which adds functionality)
  • 🧠 Refactor
  • 💄 Styles
Enregistrement.de.l.ecran.2023-12-19.a.19.02.40.mov

isOpen ? 'block' : 'pointer-events-none opacity-0',
hasPopover && 'p-lg'
)}
{...props}
Copy link
Contributor Author

@Powerplex Powerplex Dec 19, 2023

Choose a reason for hiding this comment

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

This is to enable this component to be used asChild (to merge its props with its parent props)

It's important to merge Dropdown.Popover into Dropdown.Items through asChild, as the first one is an alias of Popover.Content it has role="dialog" but it should have role="listbox" instead as the Trigger here is a combobox role.

Plus, the overflow (scrollbar) is attached to the Popover.Content, so not merging them means keyboard navigation will not auto-scroll on highlighted items (unless we do it programatically, which I prefer to avoid).

The call to getMenuProps() must be done after {...props} as we want the props from downshift to erase the one from Radix (Popover)

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (21aacb8) 97.98% compared to head (ecda003) 97.98%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1772      +/-   ##
==========================================
- Coverage   97.98%   97.98%   -0.01%     
==========================================
  Files         693      693              
  Lines        4902     4901       -1     
  Branches     1785     1785              
==========================================
- Hits         4803     4802       -1     
  Misses         99       99              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -17,7 +17,7 @@ interface TriggerProps {
const styles = cva(
[
'flex w-full cursor-pointer items-center justify-between',
'min-h-sz-44 rounded-lg bg-surface px-lg',
'min-h-sz-44 rounded-lg bg-surface text-on-surface px-lg',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We forgot to specify the text color, so whenever the Dropdown was used inside a success-container for example, it has a on-success-container text color, etc

@Powerplex Powerplex force-pushed the adjust-styles-dropdown branch from c446996 to ecda003 Compare December 20, 2023 10:06
@Powerplex Powerplex merged commit b0d7c7b into main Dec 20, 2023
10 of 11 checks passed
@Powerplex Powerplex deleted the adjust-styles-dropdown branch December 20, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants