Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-14489 Making SuggestionList component render at bottom of input on dialogs #2235

Merged
merged 3 commits into from
Mar 12, 2019

Conversation

avasconcelos114
Copy link
Contributor

Summary

Given a bug found that SuggestionLists will clip through the top of a modal body, this PR passes along the listStyle prop (as an optional prop) to make it render below the input (instead of above, which is the default behavior)

Checklist

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Has UI changes

@jasonblais jasonblais self-requested a review December 31, 2018 08:13
@jasonblais jasonblais added 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Dec 31, 2018
@hanzei
Copy link
Contributor

hanzei commented Jan 7, 2019

@jasonblais Could you please take a look at this PR when you have time? Thanks!

@jasonblais jasonblais self-assigned this Jan 7, 2019
@jasonblais jasonblais removed the Setup Old Test Server Triggers the creation of a test server label Jan 8, 2019
@mattermost mattermost deleted a comment from mattermod Jan 8, 2019
@mattermost mattermost deleted a comment from mattermod Jan 8, 2019
@mattermost mattermost deleted a comment from mattermod Jan 8, 2019
@mattermost mattermost deleted a comment from mattermod Jan 8, 2019
@jasonblais jasonblais added the Setup Old Test Server Triggers the creation of a test server label Jan 8, 2019
@jasonblais jasonblais requested a review from jwilander January 9, 2019 05:25
@jasonblais
Copy link
Contributor

Thanks @avasconcelos114, and sorry for the delay in replying.

I verified there is no change to message menus from what I can see (the menu autocomplete opens up, though there may be scenarios where we want it to open down, but that's a separate conversation)

@jwilander Would there be an easy way for a PM to test the changes by invoking dialogs set up for an integration on the spinmint?

@jwilander
Copy link
Member

I can set one up today or tomorrow, added it to my todo list

@cpanato
Copy link
Contributor

cpanato commented Jan 10, 2019

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@cpanato
Copy link
Contributor

cpanato commented Jan 10, 2019

Spintmint test server creation failed. See the logs for more information.

@jwilander
Copy link
Member

@jasonblais integration is ready and set up on the eligendi team as the slash command /dialog

@jasonblais
Copy link
Contributor

jasonblais commented Jan 10, 2019

Thanks Joram!

@avasconcelos114 I can confirm that the menu now opens down, which is a good improvement. However, if the menu is near the bottom, it'll be behind the footer element of the dialog

image

We'd want it to go in front of the footer element, like attached

image

Let me know if you have any questions, happy to clarify it.

@avasconcelos114
Copy link
Contributor Author

avasconcelos114 commented Jan 11, 2019

Yeah it would be better if the SuggestionList was rendered outside of the modal body

I'll try setting the modal's overflow to visible and confirm whether no new visual glitches are introduced (textareas clipping out of the component and the like)

Edit: As I somewhat expected, setting overflow to visible means that larger dialogs have their elements glitch out of the modal

image

Let me know if there are any recommendations on how to proceed

@jasonblais
Copy link
Contributor

@asaadmahmood Would you have suggestions on how we might proceed?

This is related to dropdown menus in interactive dialogs. In summary

A) Current - the menu always opens up, which can get clipped by the header component of the dialog:

image

B) Change in the PR - the menu always opens down, but now may get clipped by the footer component of the dialog:

image

C) Andre then tried to set the modal's overflow to be visible but this means that larger dialogs have their elements glitch out of the modal:

image

@asaadmahmood
Copy link
Contributor

@jasonblais Ideally, I would like the suggestion list to be rendered somewhere near the body tag and have it's position set by the position and width of the trigger element.
That however may be a larger ticket. Thus, what i suggest for now, if we want a quick fix is.
We set a min-height for these modals, the dropdown can then expand accordingly within the modal body.

There however should be an added functionality that if the dropdown trigger is near the end of the modal, then the modal should open upwards instead of downwards.

@jasonblais
Copy link
Contributor

What min-height would you recommend?

There however should be an added functionality that if the dropdown trigger is near the end of the modal, then the modal should open upwards instead of downwards.

👍

@asaadmahmood
Copy link
Contributor

260pxI would say.
Also, can we only have only 1 line of description for the channel descriptions inside the Channel autocomplete.
Slack does not even show the descriptions in the autocomplete.

We would also have to reduce the max-height for the suggestion list (only for the modals) to about 154px.

After those changes, it should look like this.

i-0bffbea4c7b47da2b test spinmint com_ad-1_channels_town-square

@jasonblais
Copy link
Contributor

jasonblais commented Jan 11, 2019

@asaadmahmood Note that we are using the same component for the suggestion list throughout the UI. So if we make changes here, it'll also affect other areas of the app.

In other words, max-height becomes 154px everywhere, and we only show one line of channel description in all suggestion lists.

Confirming that's what we want?

@asaadmahmood
Copy link
Contributor

@jasonblais Nops. We can target the suggestion list only in specific modals if we want, but I still am suggesting target all the modals globally.
Similarly, the one line thing or the height can be applied selectively.

I want the one line thing to be applied globally, but the height due to the height restricting in the modal applied specifically for modals.

@jasonblais
Copy link
Contributor

We can target the suggestion list only in specific modals if we want

True, but we're moving towards unified UI components across the app. So applying the one line change globally works, but engineers may be against height restrictions of the suggestion list only on modals.

@asaadmahmood
Copy link
Contributor

Then we can just make the modal body slightly larger to accommodate the default height, but that isn't what I'd recommend.
The components should be unified, however, there have to be differences based on the context in which they are used. This can precisely be done by different props per component. The component may change how it looks depending on the proptype for the component, that is quite natural.

Like the profile picture is displayed in various places, obviously we can't have the same size for the profile picture in the center channel, and in the autocompletes. The autocomplete have very limited space, so does the channel switcher, the size has to vary depending on where it is used.

Other examples include the different icons we use, the flag icon the channel header will be larger than the icon in the center channel due to space restrictions.

So in different places, the component may be the same, however, it may vary depending on the context it is being used.

@hanzei
Copy link
Contributor

hanzei commented Jan 27, 2019

What are the next steps on this ticket? @jasonblais do you need more input?

@jasonblais
Copy link
Contributor

Thanks Hanzei for the reminder and sorry Andre for the delay here.

@asaadmahmood So is the proposal to

  • display channel purpose on the same line as the channel name and handle in all channel autocompletes ?
  • reduce height of autocomplete in the interactive dialogs to 154px, but keep existing height for other autocompletes?

Andre added 2 commits March 12, 2019 09:57
- Setting position of modal body as static and rendering dropdowns outside of it

Note: positioning values changed from original, values used to position the dropdowns in the dialog are in `_about-modal.scss` (While not currently used, added `suggestion-list--top` values inside modal for possible future usage)
- Using ReactDOM to get the bottom positioning of input, and pass it to SuggestionList to place it in appropriate position when rendering below the input
@asaadmahmood
Copy link
Contributor

@avasconcelos114 @jasonblais Can we get help setting up the spinmint so I can test?

@jasonblais jasonblais added the Setup Old Test Server Triggers the creation of a test server label Mar 12, 2019
@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@mattermod
Copy link
Contributor

Spinmint test server created at: https://i-045c3178f8cdb86cc.test.spinmint.com

Test Admin Account: Username: sysadmin | Password: sysadmin

Test User Account: Username: user-1 | Password: user-1

Instance ID: i-045c3178f8cdb86cc

@asaadmahmood
Copy link
Contributor

@jasonblais Looks good, just one thing that the position of the list scrolls as I scroll in the channel.
I tried researching Discord and Slack with their post control menu. Slack disables their scroll if the menu is open. Discord behaves similarly to us, they scroll the menu with the window, thus disconnecting it from the trigger.

I think we can take this, and see what users feel, and then apply the appropriate change if needed.

@jasonblais jasonblais added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter Setup Old Test Server Triggers the creation of a test server labels Mar 12, 2019
@jasonblais
Copy link
Contributor

Sounds good - devs: This is ready to merge for 5.10. Big thanks Andre!

@avasconcelos114
Copy link
Contributor Author

Thank you all for reviewing and giving feedback :)

@asaadmahmood
Copy link
Contributor

@avasconcelos114 Really awesome work!

@hanzei hanzei merged commit ab31a59 into mattermost:master Mar 12, 2019
@mattermod
Copy link
Contributor

Spinmint test running for more than 7 days. This test server was terminated.

@lindy65 lindy65 removed the 4: Reviews Complete All reviewers have approved the pull request label Mar 14, 2019
@mattermod
Copy link
Contributor

Spinmint test running for more than 7 days. This test server was terminated.

@hanzei hanzei added the 4: Reviews Complete All reviewers have approved the pull request label Mar 19, 2019
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Mar 19, 2019
jwilander added a commit that referenced this pull request Mar 28, 2019
stevepartridge pushed a commit to stevepartridge/mattermost-webapp that referenced this pull request Mar 30, 2019
… dialogs (mattermost#2235)

Given a bug found that `SuggestionList`s [will clip through the top of a modal body](https://pre-release.mattermost.com/core/pl/wafwh5fyytgauykxdyytsqa51a), this PR passes along the `listStyle` prop (as an optional prop) to make it render below the input (instead of above, which is the default behavior)
jwilander added a commit that referenced this pull request Mar 31, 2019
jwilander added a commit that referenced this pull request Mar 31, 2019
thekiiingbob pushed a commit to thekiiingbob/mattermost-webapp that referenced this pull request Apr 2, 2019
@lindy65 lindy65 added Tests/Not Needed Does not require new release tests and removed 4: Reviews Complete All reviewers have approved the pull request labels Apr 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Not Needed Does not require new release tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants