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

search: Add jump to date feature. #25014

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

Conversation

lakshaykananiya
Copy link
Collaborator

@lakshaykananiya lakshaykananiya commented Apr 6, 2023

We currently do not have support for jumping to messages sent on a specific date for a particular stream or topic.

The approach here is to add a flatpickr popup when we click on the recipient_row_date which will make the view jump to the messages sent on the selected date when confirmed.

Used date.sent column of zerver_message table to compare the date to the selected date which comes in through anchor_date parameter that is added to /json/messages endpoint.

Continues the work of: #24357.
Fixes: #23995.

Update # 1:
Added a button in the flatpickr that pops up when we click
on the date in recipient_row_date.

A dependency is added for this: shortcut-buttons-flatpickr.

The approach is to add the plugin this dependency contains in
the flatpickr and then whenever onClose is called,
the required behavior will take place as we're passing the
beginning of the time "1970-01-01" as date through null.

Update # 2
The current version only shows the complete date in tippy.
The updated version will show a (Click to jump dates) along
with that full date to inform users about this feature.

Update # 3
Addressed feedback,

  1. Disabled future dates.
  2. Updated tippy to better represent function.
  3. Added required class to Scroll to top button.
  4. Centralised Scroll to top` button.
  5. Fixed corners poking out of the dialog.

Completed:

  • Jump to a specific date on streams/topics.
  • Jump to a specific date on all messages.
  • Functionality to jump to the top of the topic.
  • Updated tippy.
  • Frontend tests.
  • Backend tests.

Info: This PR solves one issue that is part of my GSoC proposal.

Screenshots and screen captures:
result

Latest screenshots
dark light

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.

@zulipbot zulipbot added size: XL area: message feed (UI) Buttons/UI directly in the message feed (not popovers, etc.) area: search difficult Issues which we expect to be quite difficult priority: high UX UX improvements to an existing workflow. labels Apr 6, 2023
@zulipbot
Copy link
Member

zulipbot commented Apr 6, 2023

Hello @zulip/server-search members, this pull request was labeled with the "area: search" label, so you may want to check it out!

@lakshaykananiya
Copy link
Collaborator Author

lakshaykananiya commented Apr 16, 2023

Updated version:

result

@alya
Copy link
Contributor

alya commented Apr 20, 2023

Thanks! A few thoughts:

  1. Can we add a title and some explanatory text to the top of the widget? Something like:

Jump to date

Click on any date to scroll your view.

  1. Can we disable future dates?
  2. Let's make the tooltip 2 lines:

[i.e., current tooltip]
Jump to any date

@alya
Copy link
Contributor

alya commented Apr 20, 2023

I think we'll also want to fiddle with the visuals for the "Scroll to top" button. I would try:

  1. Putting "or" and the button on separate lines, and centering both.
  2. If possible, making it look the same as other action buttons (e.g., "Send" or "Confirm" in modals).
  3. Changing the text to: "Jump to oldest message". We can play with it more, but we should try to keep the terminology consistent throughout.

@lakshaykananiya
Copy link
Collaborator Author

Addressing your first feedback,

  1. Yes, it is possible to add a heading to the top but I think that it will be a distraction more than help since the user is most probably going to know already what the dialog does and even if he doesn't, it will be easy to guess. Plus, there will be a tippy. I think it would be better to keep the dialog clean.
  2. Yes, we can. Done.
  3. Done.

@lakshaykananiya
Copy link
Collaborator Author

As for your second feedback,

  1. Does this look good? I prefer the previous one more though.

dialog

2. Done. 3. Actually, we use `Scroll to bottom` in the bottom arrow that appears when we scroll. So, maybe `Scroll to top` is right?

@lakshaykananiya
Copy link
Collaborator Author

@alya I've addressed your feedback.

@alya
Copy link
Contributor

alya commented Apr 21, 2023

If possible, making it look the same as other action buttons (e.g., "Send" or "Confirm" in modals).

Hmm, did you change the shape? The color is certainly not the same in your latest screenshot.

@alya
Copy link
Contributor

alya commented Apr 21, 2023

Yes, it is possible to add a heading to the top but I think that it will be a distraction more than help since the user is most probably going to know already what the dialog does and even if he doesn't, it will be easy to guess. Plus, there will be a tippy. I think it would be better to keep the dialog clean.

You can post in #design for more feedback.

@alya
Copy link
Contributor

alya commented Apr 21, 2023

Actually, we use Scroll to bottom in the bottom arrow that appears when we scroll. So, maybe Scroll to top is right?

This is also a fine #design topic.

@lakshaykananiya
Copy link
Collaborator Author

lakshaykananiya commented Apr 21, 2023

Hmm, did you change the shape? The color is certainly not the same as in your latest screenshot.

I changed the color after that screenshot. Sorry for the confusion.

Here's the latest screenshot,
image

@alya
Copy link
Contributor

alya commented Apr 21, 2023

OK, cool, you can post that vs. the centered button for general feedback as well.

@lakshaykananiya
Copy link
Collaborator Author

Addressed feedback and updated the PR description with latest screenshots!

baris and others added 8 commits May 8, 2023 12:04
We currently do not have support for jumping to messages sent
on a specific date for a particular stream or topic.

The approach here is to add a `flatpickr` popup when we click
on the `recipient_row_date`  which will make the view jump to
the messages sent on the selected date when confirmed.

Used `date.sent` column of `zerver_message` table to compare
the date to the selected date which comes in through
`anchor_date` parameter that is added to `/json/messages`
endpoint.

Also, added frontend and backend test along with modifying
the existing ones.

Continues the work of zulip#24357.
Fixes zulip#23995.

Co-authored-by: Lakshay Mittal <lakshaymittal225@gmail.com>
Co-authored-by: baris <e253436@metu.edu.tr>
We might want to scroll to the top of a topic or stream or
all messages but we do not have any option but to scroll
right now.

Added a button in the `flatpickr` that pops up when we click
on the date in `recipient_row_date`.

A dependency is added for this: `shortcut-buttons-flatpickr`.

The appoach is to add the plugin this dependency contains in
the `flatpickr` and then whenever `onClick` is called,
the required behavior will take place as we're passing the
beginning of the time "1970-01-01" as date through `null`.
The current version only shows the complete date in tippy.
Updated version will show a `(Click to jump dates)` along
with that full date to inform users about this feature.
Disable all future dates for flatpickr to jump to messages
of a particular date.
The top corners were poking out of the dialog in dark theme
because of an explicit background color (for dark theme only).

Added a "border-radius" for the concerned element.
@zulipbot
Copy link
Member

zulipbot commented May 9, 2023

Heads up @lakshaykananiya, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

Copy link
Member

@akshatdalton akshatdalton left a comment

Choose a reason for hiding this comment

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

Please address the feedback. @lakshaykananiya

Comment on lines +1307 to +1312
query: Union[SelectBase, Tuple[SelectBase, SelectBase]]
query, inner_msg_id_col = get_base_query_for_search(
user_profile=user_profile,
need_message=need_message,
need_user_message=need_user_message,
anchor_date=anchor_date,
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to change the base query? Just add another mapping for operator and it's method in by_method_map (check NarrowBuilder). And then you can send a query with a new operator type: near-date.

@timabbott timabbott added the completion candidate PRs with reviews that may unblock merging label Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: message feed (UI) Buttons/UI directly in the message feed (not popovers, etc.) area: search completion candidate PRs with reviews that may unblock merging difficult Issues which we expect to be quite difficult has conflicts priority: high size: XL UX UX improvements to an existing workflow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dates should be "Jump to date" buttons
5 participants