Skip to content

Hotfixes: Update SelectMenu.Item type & extend Box in Position#803

Merged
emplums merged 5 commits intorelease-19.0.0from
fix-select-item-type
May 8, 2020
Merged

Hotfixes: Update SelectMenu.Item type & extend Box in Position#803
emplums merged 5 commits intorelease-19.0.0from
fix-select-item-type

Conversation

@emplums
Copy link
Copy Markdown

@emplums emplums commented May 7, 2020

A couple of hotfixes after reviewing the 19.0.0 release:

  • Make the as prop on SelectMenu.Item optional
  • I wanted to update the SelectMenu.Modal code example to make sure folks know that the component needs to be wrapped in a relatively positioned element for the align prop to work. Which made me realize the the Position family of components should have all the same props as Box but they don't because we weren't extended Box so I fixed that.

@vercel
Copy link
Copy Markdown

vercel bot commented May 7, 2020

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/primer/primer-components/ncq2vyaap
✅ Preview: https://primer-components-git-fix-select-item-type.primer.now.sh

@emplums emplums changed the base branch from master to release-19.0.0 May 7, 2020 22:50
@emplums emplums requested a review from BinaryMuse May 7, 2020 22:50
@vercel vercel bot temporarily deployed to Preview May 7, 2020 23:08 Inactive
@emplums emplums changed the title Fix select item type Hotfixes: Update SelectMenu.Item type & extend Box in Position May 7, 2020
@vercel vercel bot temporarily deployed to Preview May 7, 2020 23:40 Inactive
Comment on lines -32 to -37
StyledSystem.PositionProps,
StyledSystem.ZIndexProps,
StyledSystem.TopProps,
StyledSystem.RightProps,
StyledSystem.BottomProps,
StyledSystem.LeftProps {}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed all of these because they are already included in the definition for StyledSystem.PositionProps

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed BaseProps because the two components that extend PositionProps (Position & Popover) already are including BaseProps in their definition

Comment on lines -277 to -279
CommonProps,
LayoutProps,
Omit<React.HTMLAttributes<HTMLDivElement>, 'color'> {}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

these are all included in BoxProps

@vercel vercel bot temporarily deployed to Preview May 7, 2020 23:43 Inactive
@emplums
Copy link
Copy Markdown
Author

emplums commented May 7, 2020

@BinaryMuse if you could review this again, I've added one more change.

I also think we should consider putting FLEX in LAYOUT in our constants file, to insure that anywhere we allow folks to use the layout props they also get the flex props. I started working on that in this branch but it got a lil hairy and I think would be fine as a follow-up after this release. It should be an internal API change with no breaking changes.

@BinaryMuse
Copy link
Copy Markdown
Contributor

One thing I forgot — we should update the Position docs

@vercel vercel bot temporarily deployed to Preview May 8, 2020 17:28 Inactive
@emplums
Copy link
Copy Markdown
Author

emplums commented May 8, 2020

@BinaryMuse good catch! 💯 more points for automating this eventually 😂

Copy link
Copy Markdown
Contributor

@BinaryMuse BinaryMuse left a comment

Choose a reason for hiding this comment

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

Sweeeeet

@emplums emplums merged commit 4470bee into release-19.0.0 May 8, 2020
@emplums emplums deleted the fix-select-item-type branch May 8, 2020 17:31
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.

2 participants