[REF-1993] link: respect is_external prop and other attributes on A tag (#2651)

* link: respect `is_external` prop and other attributes on A tag

Instead of passing all props to NextLink by default, only pass props that
NextLink understands, placing the remaining props on the Radix link

Add a test case to avoid regression of `is_external` behavior.

* Link is a MemoizationLeaf

Because Link is often rendered with NextLink as_child, and NextLink breaks if
the href is stateful outside of a Link, ensure that any stateful child of Link
gets memoized together.
This commit is contained in:
Masen Furer 2024-02-19 15:43:27 -08:00 committed by GitHub
parent 99a566f43e
commit 279e9bfa28
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 112 additions and 5 deletions

View File

@ -0,0 +1,89 @@
"""Integration tests for links and related components."""
from typing import Generator
from urllib.parse import urlsplit
import pytest
from selenium.webdriver.common.by import By
from reflex.testing import AppHarness
from .utils import poll_for_navigation
def NavigationApp():
"""Reflex app with links for navigation."""
import reflex as rx
class State(rx.State):
is_external: bool = True
app = rx.App()
@app.add_page
def index():
return rx.fragment(
rx.link("Internal", href="/internal", id="internal"),
rx.link(
"External",
href="/internal",
is_external=State.is_external,
id="external",
),
rx.link(
"External Target", href="/internal", target="_blank", id="external2"
),
)
@rx.page(route="/internal")
def internal():
return rx.text("Internal")
@pytest.fixture()
def navigation_app(tmp_path) -> Generator[AppHarness, None, None]:
"""Start NavigationApp app at tmp_path via AppHarness.
Args:
tmp_path: pytest tmp_path fixture
Yields:
running AppHarness instance
"""
with AppHarness.create(
root=tmp_path,
app_source=NavigationApp, # type: ignore
) as harness:
yield harness
@pytest.mark.asyncio
async def test_navigation_app(navigation_app: AppHarness):
"""Type text after moving cursor. Update text on backend.
Args:
navigation_app: harness for NavigationApp app
"""
assert navigation_app.app_instance is not None, "app is not running"
driver = navigation_app.frontend()
internal_link = driver.find_element(By.ID, "internal")
with poll_for_navigation(driver):
internal_link.click()
assert urlsplit(driver.current_url).path == f"/internal/"
with poll_for_navigation(driver):
driver.back()
external_link = driver.find_element(By.ID, "external")
external2_link = driver.find_element(By.ID, "external2")
external_link.click()
# Expect a new tab to open
assert AppHarness._poll_for(lambda: len(driver.window_handles) == 2)
# Switch back to the main tab
driver.switch_to.window(driver.window_handles[0])
external2_link.click()
# Expect another new tab to open
assert AppHarness._poll_for(lambda: len(driver.window_handles) == 3)

View File

@ -6,7 +6,8 @@ from __future__ import annotations
from typing import Literal
from reflex.components.component import Component
from reflex.components.component import Component, MemoizationLeaf
from reflex.components.core.cond import cond
from reflex.components.el.elements.inline import A
from reflex.components.next.link import NextLink
from reflex.utils import imports
@ -27,7 +28,7 @@ LiteralLinkUnderline = Literal["auto", "hover", "always"]
next_link = NextLink.create()
class Link(RadixThemesComponent, A):
class Link(RadixThemesComponent, A, MemoizationLeaf):
"""A semantic element for navigation between pages."""
tag = "Link"
@ -53,6 +54,9 @@ class Link(RadixThemesComponent, A):
# Whether to render the text with higher contrast color
high_contrast: Var[bool]
# If True, the link will open in a new tab
is_external: Var[bool]
def _get_imports(self) -> imports.ImportDict:
return {**super()._get_imports(), **next_link._get_imports()}
@ -70,12 +74,23 @@ class Link(RadixThemesComponent, A):
Returns:
Component: The link component
"""
is_external = props.pop("is_external", None)
if is_external is not None:
props["target"] = cond(is_external, "_blank", "")
if props.get("href") is not None:
if not len(children):
raise ValueError("Link without a child will not display")
if "as_child" not in props:
# Extract props for the NextLink, the rest go to the Link/A element.
known_next_link_props = NextLink.get_props()
next_link_props = {}
for prop in props.copy():
if prop in known_next_link_props:
next_link_props[prop] = props.pop(prop)
# If user does not use `as_child`, by default we render using next_link to avoid page refresh during internal navigation
return super().create(
NextLink.create(*children, **props), as_child=True
NextLink.create(*children, **next_link_props),
as_child=True,
**props,
)
return super().create(*children, **props)

View File

@ -8,7 +8,8 @@ from reflex.vars import Var, BaseVar, ComputedVar
from reflex.event import EventChain, EventHandler, EventSpec
from reflex.style import Style
from typing import Literal
from reflex.components.component import Component
from reflex.components.component import Component, MemoizationLeaf
from reflex.components.core.cond import cond
from reflex.components.el.elements.inline import A
from reflex.components.next.link import NextLink
from reflex.utils import imports
@ -19,7 +20,7 @@ from .base import LiteralTextSize, LiteralTextTrim, LiteralTextWeight
LiteralLinkUnderline = Literal["auto", "hover", "always"]
next_link = NextLink.create()
class Link(RadixThemesComponent, A):
class Link(RadixThemesComponent, A, MemoizationLeaf):
@overload
@classmethod
def create( # type: ignore
@ -113,6 +114,7 @@ class Link(RadixThemesComponent, A):
]
] = None,
high_contrast: Optional[Union[Var[bool], bool]] = None,
is_external: Optional[Union[Var[bool], bool]] = None,
download: Optional[
Union[Var[Union[str, int, bool]], Union[str, int, bool]]
] = None,
@ -238,6 +240,7 @@ class Link(RadixThemesComponent, A):
underline: Sets the visibility of the underline affordance: "auto" | "hover" | "always"
color_scheme: Overrides the accent color inherited from the Theme.
high_contrast: Whether to render the text with higher contrast color
is_external: If True, the link will open in a new tab
download: Specifies that the target (the file specified in the href attribute) will be downloaded when a user clicks on the hyperlink.
href: Specifies the URL of the page the link goes to
href_lang: Specifies the language of the linked document