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

EPIC-EWPP-2022-External-links #1119

Merged
merged 51 commits into from
Jun 21, 2022
Merged

EPIC-EWPP-2022-External-links #1119

merged 51 commits into from
Jun 21, 2022

Conversation

22Alexandra
Copy link
Contributor

No description provided.

nagyad and others added 30 commits March 30, 2022 13:03
EWPP-2071: Add external icon to external custom footer links.
EWPP-2070: Allow <svg> and <use> in introduction fileds.
EWPP-2069: External link icon for patterns rendering links.
/**
* {@inheritdoc}
*/
public function isExternalLink($url): bool {
Copy link
Member

Choose a reason for hiding this comment

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

We should cover this with a dedicated test, where we throw at it all sorts of different values, of different types, and check the output.

}

// If it's an external link, make sure its domain is not internal.
$internal_domain_expression = $this->configFactory->get('oe_theme_helper.internal_domains')->get('internal_domain');
Copy link
Member

Choose a reason for hiding this comment

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

We can get the expression in the constructor, as we really don't need the config factory for anything else in this service.

Let's have a property called $internalDomainExpression, and we simply fill it in in the constructor.

@22Alexandra 22Alexandra dismissed ademarco’s stale review June 21, 2022 15:28

Comments were addressed here #1125

@22Alexandra 22Alexandra merged commit f5b8d3b into 3.x Jun 21, 2022
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.

4 participants