Skip to content

[FIX] Fix permalink of message when running system with subdir#11781

Merged
ggazzo merged 5 commits intoRocketChat:developfrom
ura14h:ura14-patch-permalink
Aug 20, 2018
Merged

[FIX] Fix permalink of message when running system with subdir#11781
ggazzo merged 5 commits intoRocketChat:developfrom
ura14h:ura14-patch-permalink

Conversation

@ura14h
Copy link
Copy Markdown
Contributor

@ura14h ura14h commented Aug 15, 2018 •
edited
Loading

Closes #10732

I replaced MessageAction.getPermaLink to a new method which use FlowRouter.url.

@ura14h ura14h mentioned this pull request Aug 15, 2018
Merged
@ggazzo ggazzo added the area: ui/ux Related to UI/UX, frontend code, accessibility, and user interaction label Aug 16, 2018
@ggazzo ggazzo added this to the 0.69.0 milestone Aug 16, 2018
@madguy02
Copy link
Copy Markdown
Contributor

madguy02 commented Aug 17, 2018

@ura14h Can you fix the conflicts appearing on this branch?

@ura14h
Copy link
Copy Markdown
Contributor Author

ura14h commented Aug 17, 2018 •
edited
Loading

@madguy02 Yes. I fixed the conflict.
However, It seems that testing build failed in some code unrelated my modification.

@tassoevan tassoevan requested a review from ggazzo August 17, 2018 15:00
ggazzo
ggazzo requested changes Aug 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

@ura14h we can not use FlowRouter inside of lib because this methods are exposed both client/server side, and we dont have FlowRouter on server =/

@ura14h
Copy link
Copy Markdown
Contributor Author

ura14h commented Aug 17, 2018

@ggazzo I have confirm to work well the new code on my developing environment and a Web browser.

The original code in MessageAction.js calls getRouteLink. This method calls FlowRouter.path inside. In the my patch, I clone getRouteLink to new getURL and replace FlowRouter.path with FlowRouter.url.

If FlowRouter could not be used on server as you pointed out, I guess the original code does not work too.

ggazzo
ggazzo approved these changes Aug 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

you are right @ura14h , I did some changes just because we are working on this files... thanks for you pr

@ggazzo ggazzo merged commit d289d40 into RocketChat:develop Aug 20, 2018
@kapkoi
Copy link
Copy Markdown

kapkoi commented Aug 20, 2018 via email

@sampaiodiego sampaiodiego mentioned this pull request Aug 28, 2018
Merged
@ura14h
Copy link
Copy Markdown
Contributor Author

ura14h commented Aug 28, 2018

@ggazzo , I checked the version just released 0.96.0. But my fix is not correctly applied to this release.

Sorry for the delay in confirmation and reporting.

My first patch uses FlowRouter.url, but the final patch created by @ggazzo has been replaced by using FlowRouter.path. I guess that is the cause.

You should reopen this ticket #11781 and #10732 .

Thanks.

This was referenced Aug 28, 2018
Closed
Closed
@ura14h
Copy link
Copy Markdown
Contributor Author

ura14h commented Sep 1, 2018 •
edited
Loading

I created a new issue as to reopen this probrem.

@ura14h ura14h deleted the ura14-patch-permalink branch October 17, 2018 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggazzo ggazzo ggazzo approved these changes

Assignees

No one assigned

Labels

area: ui/ux Related to UI/UX, frontend code, accessibility, and user interaction

Projects

None yet

Milestone

0.69.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ura14h @madguy02 @kapkoi @ggazzo @tassoevan