Skip to content

Conversation

@emplums
Copy link

@emplums emplums commented Apr 23, 2020

This PR updates the Flash component with the new refresh styles ✨

I've also taken this opportunity to refactor a bit. We were previously using the scheme prop when we really should have been using variant. Sadly, this creates a breaking change but I believe it's worth it to get close to consistency. We were also naming our "schemes" based on colors, when really they should be named to describe the type of Flash (success, warning, danger, default) to aid the user in knowing which variant to use for which purpose.

These new updates also include default styling for icons included in Flash alerts - just like in Primer CSS, we set the color of any icon included in a flash alert to match the variant styling.

Screenshots

Before:

image

After:

( I removed the margin between the components to make the code more copy-able, but i'm not 100% attached to this change)
image

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.

@vercel
Copy link

vercel bot commented Apr 23, 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/i48ku2tlj
✅ Preview: https://primer-components-git-flash-refresh.primer.now.sh

@emplums emplums requested review from BinaryMuse and auareyou April 23, 2020 22:40
@vercel vercel bot temporarily deployed to Preview April 23, 2020 22:44 Inactive
@emplums emplums added the major release breaking changes label Apr 23, 2020
@emplums emplums changed the base branch from master to release-19.0.0 April 23, 2020 22:46
@emplums emplums changed the base branch from release-19.0.0 to minor April 23, 2020 22:51
@emplums emplums changed the base branch from minor to release-19.0.0 April 23, 2020 23:57
Copy link

@auareyou auareyou left a comment

Choose a reason for hiding this comment

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

Values look good! 👍

@BinaryMuse BinaryMuse mentioned this pull request Apr 27, 2020
24 tasks
Copy link
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.

Looks good. Do you mind adding a migration strategy to the release notes?

@colebemis
Copy link
Contributor

Is the border-radius of the flash container supposed to be bigger now? It looks the same in the before and after screenshots.

@vercel vercel bot temporarily deployed to Preview April 29, 2020 22:20 Inactive
@vercel vercel bot temporarily deployed to Preview April 29, 2020 22:32 Inactive
@emplums
Copy link
Author

emplums commented Apr 29, 2020

Nice catch @colebemis 🤦‍♀️ 😂

Done @BinaryMuse !

@vercel vercel bot temporarily deployed to Preview April 29, 2020 22:36 Inactive
@BinaryMuse
Copy link
Contributor

@emplums 🤔🤔🤔 worth using the new deprecation stuff for? (keep scheme for now but log a warning and support variant)

@vercel vercel bot temporarily deployed to Preview April 29, 2020 22:42 Inactive
@emplums
Copy link
Author

emplums commented Apr 29, 2020

@BinaryMuse yeah good idea! i'll add the deprecation stuff to this PR.

@vercel vercel bot temporarily deployed to Preview April 29, 2020 23:15 Inactive
@vercel vercel bot temporarily deployed to Preview April 29, 2020 23:40 Inactive
@vercel vercel bot temporarily deployed to Preview April 29, 2020 23:56 Inactive
@emplums
Copy link
Author

emplums commented Apr 29, 2020

Note: @BinaryMuse is going to add a conditional to deprecate in a follow up PR to only show the console.warn in development modes

@vercel vercel bot temporarily deployed to Preview April 30, 2020 00:20 Inactive
@vercel vercel bot temporarily deployed to Preview April 30, 2020 00:26 Inactive
@emplums
Copy link
Author

emplums commented Apr 30, 2020

@BinaryMuse I think the yarn.lock file in the 19.0.0 branch might be corrupted or something, because I have no changes to package.json in this branch and after merging in the release branch running yarn generates a ton of yarn.lock changes 🤔

@BinaryMuse
Copy link
Contributor

@BinaryMuse I think the yarn.lock file in the 19.0.0 branch might be corrupted or something, because I have no changes to package.json in this branch and after merging in the release branch running yarn generates a ton of yarn.lock changes 🤔

Hm, I pulled the release branch and ran yarn but got no changes. Can you share the diff you're getting?

@vercel vercel bot temporarily deployed to Preview May 1, 2020 22:32 Inactive
@vercel vercel bot temporarily deployed to Preview May 1, 2020 22:33 Inactive
@vercel vercel bot temporarily deployed to Preview May 1, 2020 22:41 Inactive
@emplums emplums merged commit 77ab65b into release-19.0.0 May 1, 2020
@emplums emplums deleted the flash-refresh branch May 1, 2020 22:46
@emplums emplums mentioned this pull request May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major release breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants