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

feat(components): add component context and Link component #490

Merged
merged 5 commits into from
Oct 15, 2019

Conversation

connor-baer
Copy link
Member

Closes #466.

Purpose

Some of the low-level functional components in Circuit UI need to be customizable. This is useful e.g. when your application is using a custom router and needs to use a special Link component.

Approach and changes

  • Add a ComponentsContext with a withComponents HOC and useComponents hook
  • Add a default Link component
  • Use the Link component in the Button and Sidebar components
  • Add documentation how to override the base components

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@codecov
Copy link

codecov bot commented Oct 14, 2019

Codecov Report

Merging #490 into canary will increase coverage by 0.04%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           canary     #490      +/-   ##
==========================================
+ Coverage   76.15%   76.19%   +0.04%     
==========================================
  Files         167      171       +4     
  Lines        2453     2466      +13     
  Branches      435      436       +1     
==========================================
+ Hits         1868     1879      +11     
- Misses        467      468       +1     
- Partials      118      119       +1
Impacted Files Coverage Δ
...nents/Button/components/PlainButton/PlainButton.js 100% <100%> (ø) ⬆️
...s/Button/components/RegularButton/RegularButton.js 100% <100%> (ø) ⬆️
.../components/ComponentsContext/ComponentsContext.js 100% <100%> (ø)
src/components/ComponentsContext/withComponents.js 100% <100%> (ø)
...c/components/Sidebar/components/NavItem/NavItem.js 100% <100%> (ø) ⬆️
src/util/shared-prop-types.js 88.88% <100%> (+0.42%) ⬆️
...mponents/ComponentsContext/components/Link/Link.js 100% <100%> (ø)
src/components/ComponentsContext/useComponents.js 33.33% <33.33%> (ø)
... and 2 more

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

👏 🚀 🌐

Copy link
Contributor

@marielakas marielakas left a comment

Choose a reason for hiding this comment

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

Nice! 🌞

@connor-baer connor-baer changed the title feat: add component context and Link component feat(components): add component context and Link component Oct 15, 2019
@connor-baer connor-baer merged commit 9f1ac55 into canary Oct 15, 2019
@connor-baer connor-baer deleted the feature/component-context branch October 15, 2019 11:47
@ilyanoskov
Copy link
Contributor

🎉 This PR is included in version 1.2.2-canary.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@connor-baer connor-baer removed the request for review from fernandofleury October 15, 2019 12:01
@ilyanoskov
Copy link
Contributor

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@fernandofleury
Copy link
Contributor

fernandofleury commented Oct 17, 2019

@connor-baer I think I'm missing the point (maybe long term vision) but why would the consumer use the Component context instead of importing import { Link } from 'reach-router' or Import { Link } from '@sumup/circuit-ui'

Also I might be missing the long term vision here too, but do we really want to start providing components that translate directly to a primitive? EG: Just a plain <a>.

Why would the consumer use this as Link instead of using a regular <a>?

@connor-baer
Copy link
Member Author

You make a good point here. Using the component context/primitive components from Circuit UI might not make sense for the consumer since as you rightfully pointed out, they can simply import the component directly. I didn't think about that.

However, that's only a secondary use case. The important feature here is that you can override the primitive components that Circuit UI itself uses under the hood.

Maybe it was a mistake to expose the useComponents hook and withComponents. Do you think we should remove them in v2?

@fernandofleury
Copy link
Contributor

I'm not really sure. But at first glance looks like a really complex engine to solve a single use case we have (The SideNav). Very rarely we expect the primitives to be replaced when used with the molecules we provide.

I think what would really help us here is also have a look at the component roadmap to figure out the complexity of coming updates for our components. For instance, this use case could've been initially solved by adding a linkComponent prop to the SideNav (defaulting to our Link of course)

The code is fine and well structured of course. I'm just concerned long term the Component context will just be used for the SideNav and nothing else, you know?

@connor-baer
Copy link
Member Author

connor-baer commented Oct 17, 2019

It's already being used for the Button component as well. Previously on the partner portal, the Button component was wrapped in a Link component which produced invalid and inaccessible markup:

<Link to={ROUTE.HOME}>
  <Button>Home</Button>
</Link>

// produced:

<a href="/">
  <button>Home</button>
</a>

I was able to fix it with as="span" on the Button component, but it's an extra step that's easy to miss. The website had a similar issue.

I can definitely see this being used for other components as well, e.g. a Head or Image component. It found it very helpful in my own component library, Bamboo UI. The idea of a component context was inspired by MDX.

@fernandofleury
Copy link
Contributor

Ok. I think the best way to test this out is simply using it and see how it feels. Let's give it a try!

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.

Add Link component and component context
5 participants