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

fix: Jump to message only works once #31662

Merged
merged 13 commits into from Mar 6, 2024

Conversation

SySagar
Copy link
Contributor

@SySagar SySagar commented Feb 6, 2024

fix: dynamic slug during search
feat: patch update

Proposed changes (including videos or screenshots)

Updated the slug update logic in the setMessageJumpQueryStringParameter module. Now works good.

Issue(s)

closes #31653
CORE-56
#31727

Steps to test or reproduce

  • Steps to reproduce:
  • Go to search box on top left.
  • Search any item
  • Click on jump to that message button
  • Then search something else

@SySagar SySagar requested a review from a team as a code owner February 6, 2024 10:49
Copy link

changeset-bot bot commented Feb 6, 2024

🦋 Changeset detected

Latest commit: d10e188

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 31 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/models Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@gabriellsh gabriellsh left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution!

I'll rename this PR to a more appropriate name.
I understand the reasoning behind what you did to fix the issue, but I think we can do one better. If you reset the setMessageJumQueryStringParameter.ts file, you can do the following:

const locationPathname = new URL(window.location.href).pathname as LocationPathname

This will have pretty much the same effect, but will be significantly easier to read, and will also prevent us from duplicating other query string parameters.

@SySagar
Copy link
Contributor Author

SySagar commented Feb 8, 2024

@gabriellsh I have updated your changes. Please review.

@SySagar
Copy link
Contributor Author

SySagar commented Feb 8, 2024

ok then I am reseting back all my commits and updating the changes. I guess this will be ok @gabriellsh

gabriellsh
gabriellsh previously approved these changes Feb 8, 2024
@gabriellsh gabriellsh changed the title fix: dynamic slug during search fix: Jump to messageo only works once Feb 8, 2024
@gabriellsh gabriellsh changed the title fix: Jump to messageo only works once fix: Jump to message only works once Feb 8, 2024
@gabriellsh gabriellsh added this to the 6.7 milestone Feb 8, 2024
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 54.62%. Comparing base (85fe51a) to head (d10e188).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #31662      +/-   ##
===========================================
+ Coverage    54.58%   54.62%   +0.03%     
===========================================
  Files         2280     2283       +3     
  Lines        50262    50281      +19     
  Branches     10254    10255       +1     
===========================================
+ Hits         27436    27465      +29     
+ Misses       20337    20325      -12     
- Partials      2489     2491       +2     
Flag Coverage Δ
e2e 53.63% <0.00%> (+0.05%) ⬆️
e2e-api 40.02% <ø> (+0.01%) ⬆️
unit 76.44% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@SySagar
Copy link
Contributor Author

SySagar commented Feb 8, 2024

hey @gabriellsh . I am not sure whether it will be merged or not? what would your recommend me to do?

@SySagar
Copy link
Contributor Author

SySagar commented Feb 10, 2024

any update for this pr @gabriellsh @dougfabris ?

@yash-rajpal
Copy link
Member

@SySagar Can you please address the lint issues?

@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Feb 14, 2024
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Feb 14, 2024
@SySagar
Copy link
Contributor Author

SySagar commented Feb 14, 2024

hey @gabriellsh @dougfabris I have fixed the linting issue. Can you now merge it please?

yash-rajpal
yash-rajpal previously approved these changes Feb 15, 2024
Copy link
Member

@yash-rajpal yash-rajpal left a comment

Choose a reason for hiding this comment

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

LGTM

@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Feb 15, 2024
@SySagar
Copy link
Contributor Author

SySagar commented Feb 15, 2024

hey @yash-rajpal , seems there were some more lint issue. I fixed them up.

ggazzo
ggazzo previously requested changes Feb 16, 2024
.changeset/gold-tigers-rhyme.md Outdated Show resolved Hide resolved
@SySagar
Copy link
Contributor Author

SySagar commented Feb 29, 2024

hey @ggazzo can you please approve this

@scuciatto scuciatto modified the milestones: 6.7, 7.0 Mar 5, 2024
@gabriellsh gabriellsh added the stat: ready to merge PR tested and approved waiting for merge label Mar 6, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Mar 6, 2024
@gabriellsh gabriellsh modified the milestones: 7.0, 6.7 Mar 6, 2024
@gabriellsh gabriellsh added the stat: ready to merge PR tested and approved waiting for merge label Mar 6, 2024
@kodiakhq kodiakhq bot merged commit 2c1b2c4 into RocketChat:develop Mar 6, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA tested stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dynamic search slug not working properly
6 participants