plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
427 stars 575 forks source link

Fix Size of SelectMenu in SelectWidget. #5863

Open victorchrollo14 opened 1 month ago

victorchrollo14 commented 1 month ago

Fixes #4058.

Fixes the Size of Select Menu in SelectWidget, whose height was greater than the height of parent modal as a result users had to scroll both the Select menu as well the main modal to view all the field choices.

The Issue is fixed by passing a menuPortalTarget prop into the Select Menu, that renders the select menu directly in document.body rather than the parent Modal as a result when the height of select menu is greater than the parent modal the users won't have to scroll the select menu as well as the parent modal to view all the field choices.

Screencast from 08-03-24 05:51:57 PM IST.webm

netlify[bot] commented 1 month ago

Deploy Preview for plone-components canceled.

Name Link
Latest commit 2caf15a6a2e694d49f8e25a2da06d6e1fa085f54
Latest deploy log https://app.netlify.com/sites/plone-components/deploys/661f53f65f93bc00084edd7d
netlify[bot] commented 1 month ago

Deploy Preview for volto canceled.

Name Link
Latest commit 2caf15a6a2e694d49f8e25a2da06d6e1fa085f54
Latest deploy log https://app.netlify.com/sites/volto/deploys/661f53f6de91c1000831224b
victorchrollo14 commented 1 month ago

@ichim-david. Adding overflow: hidden to the parent component disable the user from seeing all the content in the modal.

Initially I tried adding a maxMenuHeight prop to select component which fixes the issue in /controlpanel/dexterity-types/, but it doesn't work in all the places for instances you can check out /controlpanel/user where the issue persists.

Screencast from 10-03-24 06:39:56 PM IST.webm

I do agree that showing the select options below the modal looks bad. The select option is shown below the modal in several places like /controlpanel/date-and-time/, /controlpanel/site and /controlpanel/language etc.

What do you think would be better to use, should we go with the menuTargetPortal or use menuMaxHeight prop.

image

ichim-david commented 1 month ago

@ichim-david. Adding overflow: hidden to the parent component disable the user from seeing all the content in the modal.

Initially I tried adding a maxMenuHeight prop to select component which fixes the issue in /controlpanel/dexterity-types/, but it doesn't work in all the places for instances you can check out /controlpanel/user where the issue persists. Screencast.from.10-03-24.06.39.56.PM.IST.webm

I do agree that showing the select options below the modal looks bad. The select option is shown below the modal in several places like /controlpanel/date-and-time/, /controlpanel/site and /controlpanel/language etc.

What do you think would be better to use, should we go with the menuTargetPortal or use menuMaxHeight prop.

image

@victorchrollo14 I appreciate the fact that you've done research and mentioned other places where we have these react-select input and how it would look, well done, it shows a good level of commitment to thinking of a fix that would be acceptable for more than one place.

That menu does not look good if it goes outside of the content area as it is and is for sure no no in a modal. popup What is acceptable to go over the content area is the popup component that is used in the Contents section where it has some shadows and the icon that triggers it is telling you that you are about to open a menu.

I would rather have the extra scrolling rather than this look as at least that is all enclosed in the section that contains these elements and in the case of add user I find it acceptable to use scrolling to get to the options I need.

We had some changes on the contents panel where a designer added some bottom breathing area for situations where you would open the popup and it was later removed as a regression because it didn't look right for the situation where you didn't open the menu.

You can continue to experiment if you like but take into consideration that again it has to look good in order for the change to be accepted.

victorchrollo14 commented 1 month ago

hey @ichim-david . I went through most of the pages where the SelectWidget is being used. I added a maxHeightMenu prop that would keep the Select menu within the modal. This is what I found out. 1) It fixes the issue in routes /controlpanel/dexterity-types/*/schema along with a similar issue in the homepage, when we click on personal tools -> preferences -> and then try selecting a language. 2) Doesn't cause issues in other places where SelectWidget is used. at least in all the places I looked into. ( I went through all the routes, not sure If I missed any) 3) There are a few exceptions, where although we add a maxMenuHeight prop, The select menu height doesn't change, example /controlpanel/date-and-time, /controlpanel/language etc.

What do you think? Would this be fine or should we try some other alternative solutions for this problem.

image

Screencast from 11-03-24 10:09:34 PM IST.webm

Screencast from 11-03-24 10:02:37 PM IST.webm

ichim-david commented 1 month ago

hey @ichim-david . I went through most of the pages where the SelectWidget is being used. I added a maxHeightMenu prop that would keep the Select menu within the modal. This is what I found out.

1. It fixes the issue in  routes `/controlpanel/dexterity-types/*/schema` along with a similar issue in the homepage, when we click on personal tools -> preferences -> and then try selecting a language.

2. Doesn't cause issues in other places where SelectWidget is used. at least in all the places I looked into. ( I went through all the routes, not sure If I missed any)

3. There are a few exceptions, where although we add a `maxMenuHeight` prop, The select menu height doesn't change, example `/controlpanel/date-and-time`, `/controlpanel/language` etc.

What do you think? Would this be fine or should we try some other alternative solutions for this problem.

image Screencast.from.11-03-24.10.09.34.PM.IST.webm Screencast.from.11-03-24.10.02.37.PM.IST.webm

@victorchrollo14 I don't see the maxHeightMenu prop after your latest commits and force-push. look at the changes from https://github.com/plone/volto/pull/5863/files and tell me if this is the latest status

victorchrollo14 commented 1 month ago

@ichim-david. I just tried it locally earlier, I've pushed it now.

ichim-david commented 1 month ago

@ichim-david. I just tried it locally earlier, I've pushed it now.

React select has the default max-height of 300px set. You would set it to 13em which I think would be calculated from 14px as the base font size so 182px. It might be too low of a number and I'm thinking of there aren't ways to call the widget for different components with a number that makes sense for it. You also have the option of using CSS selectors for specific sections and modifying the max-height.

I'm not comfortable with thinking this is a good solution to basically say that anytime you use the select widget it has a max height of 182px instead of the default 300. Maybe 200, 250px might be a better default for us but again I am always in favor of not messing a lot with the defaults and modifying those defaults as needed per context.

If we do set the maxMenuHeight and we decide on a default that we think is more sensible than 300 we still need to ensure that someone can pass a prop and modify this value.

stevepiercy commented 1 month ago

Initially I tried adding a maxMenuHeight prop to select component which fixes the issue in /controlpanel/dexterity-types/, but it doesn't work in all the places for instances you can check out /controlpanel/user where the issue persists.

For this scenario, instead of the select menu popping downward, and assuming it uses popper.js, it should automatically detect in which direction it should pop up. Does this select menu not use popper.js? Here are docs for reference.

https://floating-ui.com/docs/react

See also related issue and pull requests:

I agree with @ichim-david that changing defaults would be a last resort because of its impacts throughout the UI.

Finally, what happened to using menuPortalTarget={document.body}?

ichim-david commented 1 month ago

Initially I tried adding a maxMenuHeight prop to select component which fixes the issue in /controlpanel/dexterity-types/, but it doesn't work in all the places for instances you can check out /controlpanel/user where the issue persists.

For this scenario, instead of the select menu popping downward, and assuming it uses popper.js, it should automatically detect in which direction it should pop up. Does this select menu not use popper.js? Here are docs for reference.

https://floating-ui.com/docs/react

See also related issue and pull requests:

* [UI Issue : dropdown popup positioning in contents page #5566](https://github.com/plone/volto/issues/5566)

* [fix: dropdown popup positioning in contents page #5571](https://github.com/plone/volto/pull/5571) (abandoned, did not pass tests)

* [Fixed #5566 dropdown popup positioning in contents page #5622](https://github.com/plone/volto/pull/5622) (awaiting review by maintainer)

I agree with @ichim-david that changing defaults would be a last resort because of its impacts throughout the UI.

Finally, what happened to using menuPortalTarget={document.body}?

@stevepiercy it's using react-select library. Using the target with document.body is not a good idea as some of these selects show up inside a modal. The modals should trap the focus and ensure that you cannot focus to things outside of the modal. If you would mount to the document.body it would add it to the end of the body and as such would go against focusing only items inside the modal.

stevepiercy commented 1 month ago

@ichim-david would this example work? https://react-select.com/advanced#portaling. I asked in https://github.com/plone/volto/issues/4058#issuecomment-1985486422, but that seems to have been lost in this PR. @victorchrollo14?

victorchrollo14 commented 1 month ago

@ichim-david. I just tried it locally earlier, I've pushed it now.

React select has the default max-height of 300px set. You would set it to 13em which I think would be calculated from 14px as the base font size so 182px. It might be too low of a number and I'm thinking of there aren't ways to call the widget for different components with a number that makes sense for it. You also have the option of using CSS selectors for specific sections and modifying the max-height.

I'm not comfortable with thinking this is a good solution to basically say that anytime you use the select widget it has a max height of 182px instead of the default 300. Maybe 200, 250px might be a better default for us but again I am always in favor of not messing a lot with the defaults and modifying those defaults as needed per context.

If we do set the maxMenuHeight and we decide on a default that we think is more sensible than 300 we still need to ensure that someone can pass a prop and modify this value.

@ichim-david yeah, I agree. putting a default value would affect different parts of the UI. if we are not aware of all the places it is being used, we might break some ui as well. react-select allows us to add custom css styling by passing in a styles prop. which we can edit in Widgets/SelectStyling.jsx. I added a max-height in there, it doesn't work as expected. The only way to add maxHeight properly is to pass the maxMenuHeight Prop.

adding maxHeight in css makes it look like below. image

In the route http://localhost:3000/controlpanel/date-and-time. Although the default max-height of menu in react-select is 300. here it exceeds that number to 501px. The default height of 300 works until the size of the options prop is 25, but when the options size is greater than 25 then the size of menu goes to 501px. What should we do about this issue? image

In order to fix the issue with schema modal. We could go with a default of 215px, which is the maximum maxMenuHeight we can put for the modal in http://localhost:3000/controlpanel/dexterity-types/*/schema such that the menu stays within the modal. Along with that we can make sure to add a prop inside SelectWidget to modify the maxMenuHeight.

victorchrollo14 commented 1 month ago

@ichim-david would this example work? https://react-select.com/advanced#portaling. I asked in #4058 (comment), but that seems to have been lost in this PR.

@victorchrollo14? I used it in the initial commits for this PR, then removed it as it wasn't really that good of a solution.