Theme-ui: Missing prop forwarding (Link component)

Created on 8 Jun 2020 · 5Comments · Source: system-ui/theme-ui

Describe the bug

A user reported a bug on my repository: https://github.com/LekoArts/gatsby-themes/issues/415

The activeClassName doesn't get passed to the Link element of Theme UI:

https://github.com/LekoArts/gatsby-themes/blob/4b576244c4f00ab1a20c84a63a91ea3f0f777303/themes/gatsby-theme-minimal-blog/src/components/navigation.tsx#L23-L25

I made sure that it's not a bug on Gatsby's side with this codesandbox:
https://codesandbox.io/s/activeclassname-rpus4

So I bisected the behavior and found two deploys of that starter project, one with theme-ui v0.2 and v0.3

v0.2: https://build-2b524a52-2d7f-4d8b-be3f-4608001dc6b9.gtsb.io/blog
v0.3: https://build-8841d060-d85a-47fe-a2a5-199d312f0ebb.gtsb.io/blog

The difference of that change was this PR where I changed Styled.a to Link from Theme UI and updated from 0.2 to 0.3:

https://github.com/LekoArts/gatsby-themes/pull/371/files#diff-2571259472ecb4e0dfebe1380900f4eb

To Reproduce

You can clone https://github.com/LekoArts/gatsby-starter-minimal-blog to see the missing active class on the items in the navigation

Expected behavior

Like in v0.2 version, a active in the className.

Additional context

bug

Source

LekoArts

👍5

Most helpful comment

Here's a simple workaround until this is fixed:

/** @jsx jsx */

import { Link as GatsbyLink } from "gatsby";
import { jsx, Link as ThemeUiLink } from "theme-ui";

export default function Link(props) {
  return (
    <ThemeUiLink as={AsLink} variant="default" {...props} __themeKey="links" />
  );
}

function AsLink(props) {
  return <GatsbyLink activeClassName="active" {...props} />;
}

Note that by default this component uses styles in theme.links.default, since (to me) it's weird for a non-Styled component to use MDX styles.

aaronadamsCA on 4 Aug 2020

👍3

All 5 comments

cc @jxnblk So that you can triage/label this

LekoArts on 26 Jun 2020

I’m currently looking into the issue I mentioned here and started with a failing test.

describe('Link', () => {
  test('components with `as` receive all props', () => {
    const Beep = props => <div {...props} />
    const json = renderJSON(<Link as={Beep} activeClassName="active" />)
    expect(json.type).toBe('div')
    expect(json.props.activeClassName).toBe('active')
  })
})

This test fails (as expected). At the moment my conclusion is that the culprit is “shouldForwardProp” in the Box component:
https://github.com/system-ui/theme-ui/blob/master/packages/components/src/Box.js#L18
When I remove that line the test is successful.

So I guess that config https://github.com/system-ui/theme-ui/blob/master/packages/components/src/Box.js#L7-L10 would also need to allow names from reach-router.

However, the PR #823 adds a new way of creating this Box component so getting this in would be the best way to solve this.

LekoArts on 26 Jun 2020

Also related to #668 -- I think the best path forward is to make some of the changes to the jsx createElement function in #823 and remove the dependencies on styled and shouldForwardProp

jxnblk on 26 Jun 2020

Here's a simple workaround until this is fixed:

/** @jsx jsx */

import { Link as GatsbyLink } from "gatsby";
import { jsx, Link as ThemeUiLink } from "theme-ui";

export default function Link(props) {
  return (
    <ThemeUiLink as={AsLink} variant="default" {...props} __themeKey="links" />
  );
}

function AsLink(props) {
  return <GatsbyLink activeClassName="active" {...props} />;
}

Note that by default this component uses styles in theme.links.default, since (to me) it's weird for a non-Styled component to use MDX styles.

aaronadamsCA on 4 Aug 2020

👍3

There isn’t a simple solution on our end for this, so we encourage using with framework-provided (Gatsby, Next, Reach/React Router, etc) Link components with a solution like the one above (https://github.com/system-ui/theme-ui/issues/990#issuecomment-668739167).