Skip to content

Use more of the screen for @-mention typeahead suggestions #4871

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

Open
alya opened this issue Jul 6, 2021 · 2 comments
Open

Use more of the screen for @-mention typeahead suggestions #4871

alya opened this issue Jul 6, 2021 · 2 comments

Comments

@alya
Copy link
Collaborator

alya commented Jul 6, 2021

At present, the @-mention typeahead suggestions dialog is fairly small. We should make it larger, as it is already unlikely the user can get useful information from other parts of the screen.

@gnprice
Copy link
Member

gnprice commented Jul 6, 2021

This would be good.

The one tricky thing here is that we don't want to have it run off the top of the screen, because then the top part of it can't be reached. We had that bug a long time ago, as #2997. We fixed that issue crudely, with a hard-coded max height, which is the behavior you see today.

From discussion at #2919 (comment) , here's some details on what I think we'll need to do in order to solve this:

[…] I think I see the reason this is tricky: it's that we have AutocompleteViewWrapper as a child of ComposeBox, when the layout we're going for really calls for making it a sibling of ComposeBox and of MessageList.

Pulling it up to be a sibling will require some tricky rewiring of how the autocomplete communicates with the input fields, as it'll be one layer farther away. I think that's totally doable, but […]

All of that still applies, except that AutocompleteViewWrapper is no longer its own component (since c4269f5.) Instead, that wrapper is this View found inside ComposeBox:

        <View style={[this.styles.autocompleteWrapper, { marginBottom: height }]}>
          <TopicAutocomplete
            isFocused={isTopicFocused}
            narrow={narrow}
            text={topic}
            onAutocomplete={this.handleTopicAutocomplete}
          />
          <AutocompleteView
            isFocused={this.state.isMessageFocused}
            selection={selection}
            text={message}
            onAutocomplete={this.handleMessageAutocomplete}
          />
        </View>

The reason it's tricky to do this as things stand is that at the place that currently lives in the React component tree, it's tucked inside the compose box and so its layout doesn't have access to things like the height of the message list. In order to give it the right layout, where it's tall when the message list is tall but short when it's short, it will need to be a sibling of MessageList -- so it needs to be invoked by ChatScreen, and not be inside ComposeBox.

That will require some refactoring, and some thinking about how to manage all the information that's in those props. As things stand, those all (except narrow) come from the ComposeBox state. The standard React approach is that if that information is needed by some component elsewhere, then it should live in the state of a component that's a common ancestor with that component.

So, one way to pursue that standard React approach here might be that ComposeBox would turn into a component that actually gets MessageList as a child, rather than a sibling.

Or a variation on that: ComposeBox itself gets split into a parent that manages the state, and a child that provides the UI with ComposeMenu, the message and topic inputs, and the send button. Then that parent gets MessageList as a child too.

A possible alternative approach would be to use a React portal, which lets a component live at a different place in the layout tree than the place in the component tree that gives it its props. Here, I think that might look like: ComposeBox and MessageList would remain siblings; their parent ChatScreen would provide a view to use as a layout container for autocomplete; but ComposeBox would still actually create the autocomplete components; and instead of putting them in its normal rendered tree, it would put them in a portal, to appear in that container. There is a version of these for RN, which @WesleyAC has been experimenting with for #4808.

@gnprice
Copy link
Member

gnprice commented Jul 8, 2021

A related point also came up in a review the other day, at #4773 (comment) :

(In fact ideally we'd have [the autocomplete widget] immediately by the message input, when autocompleting something in the message, rather than separated from it by the topic input. Or ideally ideally we'd have it right next to where the cursor is, potentially in the middle of the message input. GitHub (on the web) accomplishes that last bit, but the Zulip webapp doesn't, and it seems potentially a good bit harder, so I'm pretty content not to.)

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

No branches or pull requests

2 participants