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

Multisuggest max-height (CMEM-5868) #215

Open
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

emir89
Copy link
Contributor

@emir89 emir89 commented Nov 5, 2024

No description provided.

@emir89 emir89 requested a review from haschek November 5, 2024 10:43
Copy link
Member

@haschek haschek left a comment

Choose a reason for hiding this comment

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

The PR also misses a changelog info about the added component property.

@@ -119,6 +119,10 @@ interface MultiSelectCommonProps<T>
* If not provided, values are filtered by their labels
*/
searchPredicate?: (item: T, query: string) => boolean;
/**
* Used to calculate the max height of the dropdown.
Copy link
Member

Choose a reason for hiding this comment

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

Varname and description is a bit misleading, at least the description should state that it is the maximum height that MultiSuggestField target and its dropdown can consume together. Also the awaited range 45..100 (default value up to 100) is important to know, same for the usnit (counts of 1/100 viewport height).

src/components/MultiSelect/MultiSelect.tsx Outdated Show resolved Hide resolved
src/components/MultiSelect/MultiSelect.tsx Show resolved Hide resolved
const viewportHeight = window.innerHeight;

// Get the distance from the top of the page to the bottom of the input
const dropdownHeight =
Copy link
Member

Choose a reason for hiding this comment

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

Comment and varname are very misleading because it should be (and probably is) the height measurement of the MultiSuggestField target (not the dropdown).

const dropdownHeight =
inputRef.current.getBoundingClientRect().top + inputRef.current.getBoundingClientRect().bottom;

const availableSpace = viewportHeight - dropdownHeight;
Copy link
Member

Choose a reason for hiding this comment

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

The calculation may be not correct because with maxHeight=true I see a set --multisuggest-max-height of "100vh" what cannot be correct, it must always be smaller.

Calculation could probably also done in CSS like calc(#maxHeightOr100#vh - MultiSuggestFieldTargetHeightInPixels) (pseudo code!, replace vars by correct values). Another option could be to do set two custom properties and then do the math in the stylesheet (e.g. max-height: calc(var(--multisuggest-max-height, 45vh) - var(--eccgui-multiselectfield-currenttargetheight, 0px)). Please test this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this, neither of options worked for me. Maybe if you have some time you could also give it a try.
I improved the initial solution though, so now it seems to be working properly.

src/components/MultiSelect/MultiSelect.tsx Outdated Show resolved Hide resolved
src/components/MultiSelect/multiselect.scss Outdated Show resolved Hide resolved
src/components/index.scss Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants