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.

Sign up for GitHub

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

Jump to bottom

Use textarea-caret to place compose box typeahead near the cursor #30141

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use textarea-caret to place compose box typeahead near the cursor #30141

wants to merge 2 commits into from

Conversation

adnan-td
Copy link
Collaborator

@adnan-td adnan-td commented May 20, 2024 •
edited

Add textarea-caret package to position the typeahead close to the cursor in the textarea.

Fixes: #29974

Screenshots:

Compose box text area -

Screenshot from 2024-05-20 21-58-10

In smaller screenwidths:

Screenshot from 2024-05-20 22-05-57

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@adnan-td adnan-td mentioned this pull request May 20, 2024
Open
@zulipbot zulipbot added size: M area: compose (typeahead) Compose typeahead content and design priority: high labels May 20, 2024
@adnan-td adnan-td changed the title Issue/29974 Use textarea-caret to place compose box typeahead near the cursor May 21, 2024
@adnan-td
Copy link
Collaborator Author

@amanagr can you review this PR since you implemented tippy for typeaheads?

amanagr
amanagr reviewed May 23, 2024
View reviewed changes
web/src/bootstrap_typeahead.ts Outdated
Comment on lines 389 to 395
if (placement === "bottom-start") {
const field_height = input_element.$element.height();
assert(field_height, "Text area height property missing");
const distance = field_height - caret.top - 8;
return [caret.left, -distance];
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we just move the typeahead that uses bottom-start to use top-start too and remove this logic?

Copy link
Collaborator Author

@adnan-td adnan-td May 26, 2024 •
edited

Choose a reason for hiding this comment

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

The caret co-ordinates are relative to top left corner of text area. In bottom-start typeahead, the offset works from bottom left corner. Thus the height of reference (text-area) is used to calculate the offset from top left corner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we remove the bottom-start and only keep top-start, there's an overflow case which will arise due to removing the fallback case.

image

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah! Makes sense to have this case anyway then. I think my only desired tweak would be use a variable for 4 here to better represent what this stands for. Otherwise, LGTM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The 4 is to add a slight gap between typeahead box and caret.

This is with gap:

image

This is without gap: (feels overlapped)

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a variable for it

@amanagr
Copy link
Member

amanagr commented May 23, 2024

@adnan-td thanks for working on this! Looks great! Left a comment.

@adnan-td adnan-td force-pushed the issue/29974 branch 2 times, most recently from 9ca1c9e to c21076f Compare May 27, 2024 12:28
@amanagr
Copy link
Member

amanagr commented May 27, 2024

LGTM @adnan-td can you resovle the conflicts and push agian?

@adnan-td
Copy link
Collaborator Author

Done! Resolved the conflicts

@amanagr amanagr added the integration review Added by maintainers when a PR may be ready for integration. label May 28, 2024
@alya
Copy link
Contributor

alya commented Jun 3, 2024

There's something very strange going on with the corners of the typeahead. I think it's both top and bottom, but it's especially noticeable at the top.

Screenshot 2024-06-03 at 15 02 05

@alya
Copy link
Contributor

alya commented Jun 3, 2024

I'm sometimes seeing the typehad going down off the bottom of the window when it should be placed higher.

Screenshot 2024-06-03 at 15 05 01

@alya
Copy link
Contributor

alya commented Jun 3, 2024

Excited to see this moving forward!

@amanagr amanagr added maintainer review PR is ready for review by Zulip maintainers. and removed integration review Added by maintainers when a PR may be ready for integration. labels Jun 4, 2024
@adnan-td
Copy link
Collaborator Author

adnan-td commented Jun 4, 2024

I'm sometimes seeing the typehad going down off the bottom of the window when it should be placed higher.

Fixed this issue which occurred due to scrolling in the text area by adding scrollTop in the offset value (y-axis).

@adnan-td
Copy link
Collaborator Author

adnan-td commented Jun 4, 2024

There's something very strange going on with the corners of the typeahead. I think it's both top and bottom, but it's especially noticeable at the top.

Fixed the corners which was an overflow issue.

image

@adnan-td
typeahead: Fix typeahead position using carets.
eca495b 
Utilize textarea-caret package to offset the typeahead position.
Now typeahead is close to the cursor.

Fixes: zulip#29974
@alya
Copy link
Contributor

alya commented Jun 5, 2024

@amanagr Any additional feedback before this goes to integration review? I haven't re-tested.

@amanagr amanagr added integration review Added by maintainers when a PR may be ready for integration. and removed maintainer review PR is ready for review by Zulip maintainers. labels Jun 6, 2024
@amanagr
Copy link
Member

amanagr commented Jun 6, 2024

Looks good to me as well. Tested this is working well for compose box and message edit box.

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

@amanagr amanagr amanagr left review comments

Assignees
No one assigned
Labels
area: compose (typeahead) Compose typeahead content and design integration review Added by maintainers when a PR may be ready for integration. priority: high size: M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Position compose typeaheads near cursor
4 participants
@adnan-td @amanagr @alya @zulipbot