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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix order to escape proposal content #5367

Merged
merged 2 commits into from Sep 27, 2019
Merged

Fix order to escape proposal content #5367

merged 2 commits into from Sep 27, 2019

Conversation

oriolgual
Copy link
Contributor

馃帺 What? Why?

We need to escape & strip before executing the hashtag renderer to maintain the same workflow as before.

馃搶 Related Issues

@oriolgual oriolgual requested a review from a team as a code owner September 26, 2019 14:29
Copy link
Contributor

@tramuntanal tramuntanal left a comment

Choose a reason for hiding this comment

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

I find it ok, which was the exact problem?

Also added a suggestion.

end
text

Anchored::Linker.auto_link(text, target: "_blank", rel: "noopener")
Copy link
Contributor

Choose a reason for hiding this comment

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

Today I was thinking to open a new PR because text links are always promoted to anchors. But there's already a links flag that we can reuse and make the behavior more consistent.

What's your opinion, do you want me to open a new PR with this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Anchored::Linker.auto_link(text, target: "_blank", rel: "noopener")
text = Anchored::Linker.auto_link(text, target: "_blank", rel: "noopener") if links
text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense @tramuntanal!

end
text

Anchored::Linker.auto_link(text, target: "_blank", rel: "noopener")
Copy link
Contributor

Choose a reason for hiding this comment

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

same as before...

@oriolgual
Copy link
Contributor Author

I find it ok, which was the exact problem?

Also added a suggestion.

As you can see at MetaDecidim, there's some double escaping and apostrophes are being displayed as html entities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants