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

[Styles] Prefer Anonymous Default Export Pattern #903

Merged
merged 12 commits into from
Apr 2, 2021

Conversation

michaeljaltamirano
Copy link
Contributor

@michaeljaltamirano michaeljaltamirano commented Mar 31, 2021

  1. The majority of this diff is straightforward: it moves our export const styled component exports to conform to our anonymous export default preference.
    • The motivation for this is that we are trying to restrict the API of this library. One of the leakier parts of radiance-ui is that anyone can import these styled components directly from radiance-ui/lib. This change requires people to import the style file default export first, which makes it less likely people will do that.
      • In the future we should actually strictly prevent this, perhaps as part of or after Consistent bundling #831.
      • We want this restriction because if there are designs outside of existing patterns we should augment the component library to reflect the design and implementations, rather than work around existing limitations.
      • In the meantime setting up our library this way means that when other folks add new components they can see that this is the convention.
  2. As a result of this convention, this PR overhauls the organization in src/shared-components/button. Right now it's disorderly: the Button component itself imports (and exports) the other Button components, its style file is mostly shared styles, and anecdotally making changes to these components is always thorny due to the interconnectedness.
    • This PR moves the Button code that concerns the Button components to the /components sub-directory, to be a sibling of our other Button-like components. It maintains files at src/shared-components/button for shared styles and types, and exports the 5 buttons that src/shared-components/index.ts also exports to make it available to the bundle.

The size changes in the bundle is mostly because the non-production source map slug is longer (in the Accordion, it's 5066 characters vs. 4950), which wouldn't be reflected in our production bundles. Rollup also moves the anonymous default export into a var Style declaration that creates a tiny bit of additional overhead.

Due to the changing export pattern, releasing this will require a breaking change in order to also remove imports like import { OuterContainer } from 'radiance-ui/lib/shared-components/selectorButton/style'; and find some other way to achieve the same behavior.

@snags88 snags88 temporarily deployed to curology-radiance-pr-903 March 31, 2021 23:12 Inactive
@snags88 snags88 temporarily deployed to curology-radiance-pr-903 March 31, 2021 23:34 Inactive
@@ -2,26 +2,17 @@ import React from 'react';
import PropTypes from 'prop-types';
import { useTheme } from 'emotion-theming';

import Loader from './shared-components/loader';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only change in this file is the relative import paths changing, and removing the importing/exporting of other non-Button buttons

@@ -1,8 +1,8 @@
import React from 'react';

import { RoundButton } from '../../button';
import { RoundButton } from '../../button/components/roundButton';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes incidentally make exports like this clearer, too

Copy link
Contributor

@daigof daigof left a comment

Choose a reason for hiding this comment

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

👍 I love the standarization, it looks great

@daigof daigof self-requested a review April 1, 2021 17:22
Copy link
Contributor

@daigof daigof left a comment

Choose a reason for hiding this comment

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

:thumbs forgot to click on Approve

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.

3 participants