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

Migrate grid components to TypeScript #1504

Merged
merged 4 commits into from
Apr 5, 2022
Merged

Conversation

connor-baer
Copy link
Member

@connor-baer connor-baer commented Apr 4, 2022

Addresses #516.

Purpose

When I started to remove the use of lodash in Circuit UI, I quickly found that it was used most heavily in the grid components. The current implementation is quite complicated, so I didn't feel comfortable refactoring it without the safety of types.

Approach and changes

  • Move all grid components (Grid, Row, and Col) into a single folder and migrate them to TypeScript
  • Refactor the grid util functions for clarity, to remove the use of most lodash methods, and to be used consistently across all grid components

ℹ️ API change: The Col component's span and skip props now accept numbers or numeric strings, even when nested in a breakpoints object. Here are some examples:

<Col span={2} />
<Col span="2" />
<Col span={{ default: 2, kilo: "4" }} />

Previously, a simple value would have to be a numeric string, while a breakpoint object value would work with either. Since this is an extension of the API, it is not a breaking change.

Definition of done

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

@changeset-bot
Copy link

changeset-bot bot commented Apr 4, 2022

🦋 Changeset detected

Latest commit: e3af387

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Apr 4, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/4zY8hxSpjWWoM9JbMiWDZFBjsheQ
✅ Preview: https://oss-circuit-ui-git-feature-grid-typescript.sumup-vercel.app

@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #1504 (e3af387) into main (bbd20fd) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1504      +/-   ##
==========================================
+ Coverage   92.33%   92.44%   +0.11%     
==========================================
  Files         202      202              
  Lines        4213     4210       -3     
  Branches     1351     1353       +2     
==========================================
+ Hits         3890     3892       +2     
+ Misses        303      298       -5     
  Partials       20       20              
Impacted Files Coverage Δ
packages/circuit-ui/components/Grid/constants.ts 100.00% <ø> (ø)
packages/circuit-ui/components/Grid/Col/Col.ts 100.00% <100.00%> (ø)
packages/circuit-ui/components/Grid/Grid/Grid.ts 100.00% <100.00%> (ø)
packages/circuit-ui/components/Grid/Row/Row.ts 100.00% <100.00%> (ø)
packages/circuit-ui/components/Grid/utils.ts 100.00% <100.00%> (ø)
packages/circuit-ui/util/helpers.ts 100.00% <100.00%> (ø)
packages/circuit-ui/util/type-check.ts 100.00% <100.00%> (ø)

Comment on lines +7 to +9
max-width: 880px;
padding-left: calc(16px / 2);
padding-right: calc(16px / 2);
Copy link
Member Author

Choose a reason for hiding this comment

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

This addition to the default styles is caused by the consistent use of composeBreakpoints in all grid components. It has no effect since it's overridden by the breakpoint styles below which have higher specificity.

Comment on lines +6 to +7
margin-left: calc(-16px / 2);
margin-right: calc(-16px / 2);
Copy link
Member Author

Choose a reason for hiding this comment

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

This addition to the default styles is caused by the consistent use of composeBreakpoints in all grid components. It has no effect since it's overridden by the breakpoint styles below which have higher specificity.

import type { Theme } from '@sumup/design-tokens';
import type { SerializedStyles } from '@emotion/react';
import { css } from '@emotion/react';
import { entries } from 'lodash/fp';
Copy link
Member Author

@connor-baer connor-baer Apr 4, 2022

Choose a reason for hiding this comment

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

This remaining lodash method is required for compatibility with IE11 and can be removed in v5.

@connor-baer connor-baer marked this pull request as ready for review April 5, 2022 05:55
@connor-baer connor-baer requested a review from a team as a code owner April 5, 2022 05:55
@connor-baer connor-baer requested review from robinmetral and removed request for a team April 5, 2022 05:55
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.

Nice! Looks good to me, GitHub didn't show a super useful diff because of the file renaming but I trust Chromatic and our unit tests 👍

@connor-baer connor-baer merged commit 467c3a8 into main Apr 5, 2022
@connor-baer connor-baer deleted the feature/grid-typescript branch April 5, 2022 12:35
@github-actions github-actions bot mentioned this pull request Apr 5, 2022
@connor-baer connor-baer mentioned this pull request Apr 5, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants