-
Notifications
You must be signed in to change notification settings - Fork 122
fix(space-widget): links in messages should open in new tab #1431
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(space-widget): links in messages should open in new tab #1431
Conversation
// eslint-disable-reason content is generated from elsewhere in the app | ||
// eslint-disable-next-line react/no-danger | ||
dangerouslySetInnerHTML={{__html: content}} | ||
dangerouslySetInnerHTML={{__html: content.replace(/<a(?![^>]*target="_blank"|\s*href\s*=\s*["'](?:mailto:|tel:))([^>]*)>/gi, '<a target="_blank"$1>')}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is used when there is an @mention present in the message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does @mentioned links take to today in the widgets? Should we have a test case for that one as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @mentioned links aren't clickable. They add a new tag called <spark-mention />
and wrap the person data around it. Whenever a mention is present in a message, this component gets used instead of react-component-activity-text. So I had to add the replacement logic here as well.
eg:
<spark-mention data-object-type="person" data-object-id="801bb984-343b-4f6b-97ae-d13c91d4b877">John Doe</spark-mention>
it('renders clean links properly', () => { | ||
const component = renderer.create(<ActivityText content={cleanLink} />); | ||
|
||
expect(component).toMatchSnapshot(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have tests for cases where there is already _blank
or mailto:
or tel:
in the anchor tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests
// eslint-disable-reason content is generated from elsewhere in the app | ||
// eslint-disable-next-line react/no-danger | ||
dangerouslySetInnerHTML={{__html: content}} | ||
dangerouslySetInnerHTML={{__html: content.replace(/<a(?![^>]*target="_blank"|\s*href\s*=\s*["'](?:mailto:|tel:))([^>]*)>/gi, '<a target="_blank"$1>')}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does @mentioned links take to today in the widgets? Should we have a test case for that one as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if tests are there already and are required for the @mentions
case. Please add if required. Approving
Problem:
When the space widget is loaded inside of an iframe, the links in the messages when clicked are restricted from being loaded.
Solution:
Add target="_blank" attribute to open these links in a new tab
JIRA: SPARK-556112