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

Dropdowns should have a max-height and a scroll bar #6105

Closed
FelixMalfait opened this issue Jul 3, 2024 · 23 comments · Fixed by #8055 or #8167
Closed

Dropdowns should have a max-height and a scroll bar #6105

FelixMalfait opened this issue Jul 3, 2024 · 23 comments · Fixed by #8055 or #8167
Assignees
Labels
for experienced contributor prio: high scope: front Issues that are affecting the frontend side only

Comments

@FelixMalfait
Copy link
Member

I was shadowing a client that had created lots of field and the Filter dropdown was bigger than their screen's height.
We might (or might not?) want to set a max-height.
Also we want to make sure there's a scroll bar upon scroll (when I added a max-height directly in Chrome, the dropdown become scrollable but I couldn't see any scroll bar)

cc @Bonapara for recommandation

@FelixMalfait FelixMalfait added type: bug scope: front Issues that are affecting the frontend side only labels Jul 3, 2024
Copy link
Contributor

greptile-apps bot commented Jul 3, 2024

To address the issue of dropdowns being larger than the screen height and ensuring a visible scrollbar, follow these steps:

  1. Update DropdownMenuItemsContainer.tsx:
const StyledDropdownMenuItemsExternalContainer = styled.div<{
  hasMaxHeight?: boolean;
}>`
  --padding: ${({ theme }) => theme.spacing(1)};

  align-items: flex-start;
  display: flex;

  flex-direction: column;
  gap: 2px;
  height: 100%;
  max-height: ${({ hasMaxHeight }) => (hasMaxHeight ? '188px' : 'none')};
  overflow-y: auto;
  overflow-y: scroll; /* Ensure scrollbar is visible */

  padding: var(--padding);

  width: calc(100% - 2 * var(--padding));
`;
  1. Update DropdownMenu.tsx:
const StyledDropdownMenu = styled.div<{
  disableBlur?: boolean;
  disableBorder?: boolean;
  width?: `${string}px` | `${number}%` | 'auto' | number;
}>`
  backdrop-filter: ${({ theme, disableBlur }) =>
    disableBlur ? 'none' : theme.blur.medium};

  color: ${({ theme }) => theme.font.color.secondary};

  background: ${({ theme, disableBlur }) =>
    disableBlur
      ? theme.background.primary
      : theme.background.transparent.primary};

  border: ${({ disableBorder, theme }) =>
    disableBorder ? 'none' : `1px solid ${theme.border.color.medium}`};
  border-radius: ${({ theme }) => theme.border.radius.md};

  box-shadow: ${({ theme }) => theme.boxShadow.strong};

  display: flex;

  flex-direction: column;
  z-index: 1;
  width: ${({ width = 160 }) =>
    typeof width === 'number' ? `${width}px` : width};
  max-height: 188px; /* Set max-height */
  overflow-y: auto; /* Ensure scrollbar is visible */
`;

These changes will ensure that the dropdowns have a max-height and a visible scrollbar when the content exceeds the specified height.

References

/packages/twenty-front/src/modules/ui/layout/dropdown/components/DropdownMenuItemsContainer.tsx
/packages/twenty-front/src/modules/ui/layout/dropdown/components/DropdownMenu.tsx
/packages/twenty-front/src/modules/ui/input/components/Select.tsx
/packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx

About Greptile

This response provides a starting point for your research, not a precise solution.

Help us improve! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

Ask Greptile · Edit Issue Bot Settings

@Bonapara
Copy link
Member

Bonapara commented Jul 3, 2024

The max-height should be automatically determined based on the screen height. We should always maintain a minimum 20px bottom margin between the menu and the screen bottom:

image

image

@cooldude6000
Copy link

Is this issue open ? Can i take it?

@Bonapara
Copy link
Member

Bonapara commented Jul 3, 2024

Sure! Thanks for contributing @gowreesh369

@Faisal-imtiyaz123
Copy link
Contributor

@cooldude6000 are you working here?

@Bonapara
Copy link
Member

Just assigned you @Faisal-imtiyaz123, thanks for contributing!

@Faisal-imtiyaz123
Copy link
Contributor

@Bonapara Thanks for your quick response as always!

@Faisal-imtiyaz123
Copy link
Contributor

Faisal-imtiyaz123 commented Jul 28, 2024

@Bonapara What should be scroll bar dimensions and color?

@Bonapara
Copy link
Member

Hi @Faisal-imtiyaz123, we should use the default scroll bar! Thanks for handling this one ;)

@Faisal-imtiyaz123
Copy link
Contributor

@Bonapara The reason scroll-bar is not visible is because its background gets set to transparent.
Screenshot 2024-07-29 at 3 44 45 PM
Now will have to override it . Which color should be used?

@Bonapara
Copy link
Member

I see @Faisal-imtiyaz123, we should remove it indeed!

@Faisal-imtiyaz123
Copy link
Contributor

@Bonapara So will you provide a different color for the scroll-bar ryt? Is that what you mean. Pardon me if i misunderstood.

@Bonapara
Copy link
Member

@Faisal-imtiyaz123 I think we should remove the "background-color" property for the scrollbar so the default browser color applies automatically.

@Faisal-imtiyaz123
Copy link
Contributor

Faisal-imtiyaz123 commented Jul 29, 2024

@Bonapara I think the best fix to get a default scroll bar and its default behaviour that it shows on scroll is to remove the scroll bar styles from the DefaultLayout.tsx

@Bonapara
Copy link
Member

@Faisal-imtiyaz123, where else are they used apart from menus?

@Faisal-imtiyaz123
Copy link
Contributor

Faisal-imtiyaz123 commented Jul 30, 2024

In DefaultLayout.tsx only these styles are applied globally.
Screenshot 2024-07-30 at 3 07 37 PM
These aren't then overriden anywhere. I think we should remove these.

@Bonapara
Copy link
Member

maybe @lucasbordeau will have an input

@lucasbordeau
Copy link
Contributor

@Faisal-imtiyaz123 Here's how to do it :

  • All dropdowns are using the DropdownMenuItemsContainer component
  • This component has a hasMaxHeight property which allows to add a scroll behavior
  • Right now the max-height is hard coded at 188px

We would like to refactor all the dropdowns of the app so that they stop at 20px from the bottom of the viewport and remove this hasMaxHeight props from DropdownMenuItemsContainer and the max-height from dropdown menu container.

Instead we should implement the size middleware from floating-ui : https://floating-ui.com/docs/size

Put the availableHeight in a react state and use it in a new props height in DropdownMenu.

From here it should be straightforward to adjust CSS in sub components to have all the dropdowns behaving the same.

@FelixMalfait
Copy link
Member Author

When merging a PR for this, make sure it also solves #6864

@lucasbordeau lucasbordeau self-assigned this Oct 22, 2024
@harshrajeevsingh
Copy link
Contributor

@FelixMalfait I got this working. Can I raise a PR?

@FelixMalfait
Copy link
Member Author

@harshrajeevsingh of course thank you!

martmull added a commit that referenced this issue Oct 28, 2024
Fixes: #6105 

### Problem

- The dropdown gets clipped when the number of filters increases. 

### Solution

- Added scroll property to the ```DropdownMenu``` 
- Added size middleware to the floating UI hook.
- Provided padding of 20px to the size middleware, so that it maintains
distance from the bottom of the screen.

[Screencast from 2024-10-25
13-47-04.webm](https://github.com/user-attachments/assets/c2315ee2-6092-4c4a-8126-dba7ac3bf49b)

---------

Co-authored-by: martmull <[email protected]>
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Product development ✅ Oct 28, 2024
@martmull martmull reopened this Oct 29, 2024
@martmull
Copy link
Contributor

Reopening as the fix as been reverted due to side effects @harshrajeevsingh

@martmull
Copy link
Contributor

@harshrajeevsingh you can create a new branch linked to that issue if you want. If you need to add back your previous fix, just revert the Revert "fix: dropdowns should have a max-height & padding from screen" commit in your branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment