mattermost-webapp icon indicating copy to clipboard operation
mattermost-webapp copied to clipboard

[MM-36280] Show recent searches in Search popover (#18841)

Open goulinkh opened this issue 1 year ago • 8 comments

Summary

Show recent searches of the user is the search bar.

QA

  • Make a search query
  • Reopen the search bar
  • Select a search type: file or message
  • Make sure that the recent searches are shown in the list
  • Make sure that the list is navigable with the mouse also the keyboard (up and down arrows)
  • Make sure that the behavoir of the recent search items matches the description in Figma

Ticket Link

Related Pull Requests

  • This PR in the server side could be done seperately and once it's merged we can add the remove button (https://github.com/mattermost/mattermost-server/pull/21148)

Screenshots

before after
image image

Release Note

Show 5 most recent searches in the search popover

goulinkh avatar Oct 02 '22 17:10 goulinkh

Hello @goulinkh,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

mattermod avatar Oct 02 '22 17:10 mattermod

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

mattermod avatar Oct 02 '22 17:10 mattermod

Creating a new SpinWick test server using Mattermost Cloud.

mm-cloud-bot avatar Oct 06 '22 16:10 mm-cloud-bot

Mattermost test server created! :tada:

Access here: https://mattermost-webapp-pr-11244.test.mattermost.cloud

Account Type Username Password
Admin sysadmin Sys@dmin123
User user-1 User-1@123

mm-cloud-bot avatar Oct 06 '22 16:10 mm-cloud-bot

Thanks @hmhealey and @nevyangelova for the detailed review and explanation especially for the scroll behavior :slightly_smiling_face: I've fixed all the proposed changes feel free to give it another go!

goulinkh avatar Oct 17 '22 22:10 goulinkh

New commit detected. SpinWick will upgrade if the updated docker image is available.

mm-cloud-bot avatar Oct 18 '22 08:10 mm-cloud-bot

Mattermost test server updated with git commit 4bfbc94a0321ffaf2bdde37e9da4a36c62ad1568.

Access here: https://mattermost-webapp-pr-11244.test.mattermost.cloud

mm-cloud-bot avatar Oct 18 '22 08:10 mm-cloud-bot

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

mattermod avatar Oct 31 '22 01:10 mattermod

@hmhealey Thanks for the review! I've addressed your comments, please let me know if there is anything else to change!

goulinkh avatar Nov 20 '22 03:11 goulinkh

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

mattermod avatar Dec 04 '22 01:12 mattermod

LG, just update the failing specs:)

nevyangelova avatar Dec 06 '22 12:12 nevyangelova

LG, just update the failing specs:)

nevyangelova avatar Dec 06 '22 12:12 nevyangelova

@nevyangelova I couldn't figure out the reason of the test failure, sorry!

goulinkh avatar Dec 06 '22 20:12 goulinkh

/update-branch

nevyangelova avatar Dec 06 '22 21:12 nevyangelova

Error trying to update the PR. Please do it manually.

mattermod avatar Dec 06 '22 21:12 mattermod

please update with latest master from our repo and rerun again :)

nevyangelova avatar Dec 06 '22 21:12 nevyangelova

@esethna Note that deleting recent searches isn't possible yet on this test server because the corresponding server PR hasn't been merged yet, but we can test the rest of the functionality

hmhealey avatar Dec 08 '22 21:12 hmhealey

@goulinkh It looks like the failing test is an actual failure and not something that was fixed by merging the latest changes in. Would you mind taking a look at that?

hmhealey avatar Dec 08 '22 21:12 hmhealey

@goulinkh It looks like the failing test is an actual failure and not something that was fixed by merging the latest changes in. Would you mind taking a look at that?

Should be good to go!

goulinkh avatar Dec 12 '22 08:12 goulinkh

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

mattermod avatar Dec 24 '22 01:12 mattermod

Forwarding to QA review @jgilliam17. Feel free to ping me if needed

esethna avatar Jan 09 '23 19:01 esethna

/e2e-test

jgilliam17 avatar Jan 09 '23 19:01 jgilliam17

/update-branch

jgilliam17 avatar Jan 16 '23 22:01 jgilliam17

/e2e-test

jgilliam17 avatar Jan 17 '23 01:01 jgilliam17

Successfully triggered E2E testing! GitLab pipeline | Test dashboard

mattermost-build avatar Jan 17 '23 01:01 mattermost-build

/update-branch

jgilliam17 avatar Jan 21 '23 00:01 jgilliam17

/e2e-test

jgilliam17 avatar Jan 23 '23 22:01 jgilliam17

Successfully triggered E2E testing! GitLab pipeline | Test dashboard

mattermost-build avatar Jan 23 '23 22:01 mattermost-build

Thanks @goulinkh Really looking forward to having recent search history Seeing few small issues

  1. "Recent Searches" font style and opacity doesn't match Figma @asaadmahmood can you please provide direction
Screen Shot 2023-01-24 at 2 17 08 PM
  1. "Recent searches" is not vertically centered, the whole header seem to be a bit larger than it needs to be. There is a lot of white space.

jgilliam17 avatar Jan 24 '23 19:01 jgilliam17

Apologies I accidentally clicked on close while adding my comment

jgilliam17 avatar Jan 24 '23 19:01 jgilliam17

Continued:

  1. When searching for Files, we are not displaying "Recent Files". Let me know if this is supposed to be included in this PR.
  2. When selecting one of the recents searches, new search entry was created. I expected search term to be only moved up on the list based on recency, instead new item is created e.g. search for "pdf", select "pdf" from the list of recent searches observed: new "pdf" search is added to the list Screen Shot 2023-01-24 at 2 41 51 PM
  3. Clicking on x to clear the search, only removes label, Messages or Files, and leaves search term. We should clear the whole field.

https://user-images.githubusercontent.com/52937121/214395259-1cb4eb7d-53ae-49a3-946c-98aa21b123fd.mov

E2E report looks good, no PR related failures. @goulinkh Would you be open to adding a Cypress test for the search history?

jgilliam17 avatar Jan 24 '23 19:01 jgilliam17

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

mattermost-build avatar Feb 04 '23 01:02 mattermost-build

/update-branch

stevemudie avatar Feb 17 '23 20:02 stevemudie

Error trying to update the PR. Please do it manually.

mattermost-build avatar Feb 17 '23 20:02 mattermost-build