Skip to content

Conversation

@emplums
Copy link

@emplums emplums commented Mar 17, 2020

It looks like Grid was missing type definitions 😬

Closes #718

Merge checklist

  • Added or updated TypeScript definitions (index.d.ts) if necessary
  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@emplums emplums added the minor release new features label Mar 17, 2020
@emplums emplums requested a review from dmarcey March 17, 2020 07:03
@vercel
Copy link

vercel bot commented Mar 17, 2020

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

🔍 Inspect: https://zeit.co/primer/primer-components/akaflcu6d
✅ Preview: https://primer-components-git-grid.primer.now.sh

@emplums emplums changed the base branch from master to minor March 17, 2020 07:03
Copy link
Contributor

@dmarcey dmarcey left a comment

Choose a reason for hiding this comment

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

👍 Looks good. One small suggestion about the placement of the interface within the declare file.

Comment on lines +192 to +194
export interface GridProps extends BoxProps, StyledSystem.GridProps, Omit<React.HTMLAttributes<HTMLSpanElement>, 'color'> {}

export const Grid: React.FunctionComponent<GridProps>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we put this above the CounterLabelProps? Right now it's separating the CounterLabelProps from the export const CounterLabel... line.

Copy link
Author

Choose a reason for hiding this comment

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

yes good call! This file need some alphabetizing love as well at some point!

@vercel vercel bot temporarily deployed to Preview March 18, 2020 00:44 Inactive
@emplums emplums merged commit 2131c01 into minor Mar 18, 2020
@emplums emplums deleted the grid branch September 16, 2020 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor release new features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@primer/components has no exported member Grid

3 participants