Skip to content

Conversation

@emplums
Copy link

@emplums emplums commented Mar 20, 2020

This PR fixes a few issues brought up by another GitHub team:

  • Dropdown title type definition is clashing with the CommonProps types that it extends. It was also on DropdownMenu when it should have just been on Dropdown.

    BaseTypes extends React.Props<any> and adds the title attribute as an acceptable type. I'm honestly not sure why title is included in BaseTypes 🤔 but because it's set as only a string it conflicts with the title prop in Dropdown. I ended up needing to redefine all the types we want from BaseTypes and CommonTypes on Dropdown to get around this.

  • Changed the theme call in UnderlineNav to access colors.accent instead of colors.orange[5] to allow users of the component to override this color if needed by changing the accent color in the theme

@vercel
Copy link

vercel bot commented Mar 20, 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/f5phi6x4q
✅ Preview: https://primer-components-git-dropdown-types.primer.now.sh

@emplums emplums changed the base branch from master to minor March 20, 2020 19:56
@emplums emplums changed the title Hotline Changes Hotline Changes (update Dropdown type & add accent color to theme) Mar 20, 2020
@vercel vercel bot temporarily deployed to Preview March 20, 2020 21:49 Inactive
@vercel vercel bot temporarily deployed to Preview March 20, 2020 21:54 Inactive
@emplums emplums requested a review from T-Hugs March 20, 2020 21:58
@vercel vercel bot temporarily deployed to Preview March 20, 2020 22:42 Inactive
@emplums emplums merged commit 7192e00 into minor Mar 20, 2020
@emplums emplums deleted the dropdown-types branch March 20, 2020 22:45
@emplums emplums changed the title Hotline Changes (update Dropdown type & add accent color to theme) Update Dropdown type & add accent color to theme Mar 23, 2020
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