-
Notifications
You must be signed in to change notification settings - Fork 823
Forms: use Badge component for badges in Forms #44347
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
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
LGTM! |
e29cd63
to
c300974
Compare
34b20ff
to
60a7c05
Compare
...ages/forms/src/blocks/contact-form/components/jetpack-integrations-modal/salesforce-card.tsx
Outdated
Show resolved
Hide resolved
@@ -1,4 +1,5 @@ | |||
.integration-card { | |||
--wp-admin-theme-color: #000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when testing, I found a weird issue the the "Plugin needs install" button was gray (like the screenshots in the PR description) when on the central integrations dashboard... but blueish when on the integrations modal in the block editor.

After some digging, the reason is that the badge color is based on a variable --wp-admin-theme-color
, which for some reason has different values on a normal admin page (#000000) vs the block editor (#007cba).
This is for badges with intent = info.
I thought about changing the intent for these to "default", which is consistently gray in both places, but then we lose the badge icon. In the component library, when the badge has 'default', the icon is dropped.
As a demo, I pushed this small change which overrides the --wp-admin-theme-color in the editor, but I actually don't think this is a good idea. Overriding this will produce other kinds of potential inconsistency.
In any case, with this change, the color is gray as expected. Also note sales force is the same gray color, also with icon, because I updated it's intent to be info as well. Whatever intent we use, we should likely use the same intent for the 'Plugin needs install' and Salesforce 'Enter organization ID' badges.

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, even in earlier designs, the gray style badges didn't have icons. So I updated these back to the 'default' variant, which is always gray. And that allowed me to remove the 'bad' css override above.
The final looks like this in the editor:
And like this in the dashboard:
Those both look great IMO.
60a7c05
to
9ad8740
Compare
<Badge intent="warning" className="integration-card__setup-badge"> | ||
{ __( 'Needs connection', 'jetpack-forms' ) } | ||
</Badge> | ||
) ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased this on top of some other work to change the order of the badges on mobile.
The main difference after addressing merge conflicts is we had to keep the class names like integration-card__connected-badge
so that we could continue to apply the flex order changes on desktop vs mobile.
Likewise, in the CSS we kept the rules applying the order for those classes.
Uses Badge component from the new Automattic UI library. Badge is the same as core-badge.
Tried to apply sensible badge colours everywhere but not sure if I got it right. 😅 @edanzer can you help review?
Grey neutral for items which are fine to ignore if they aren't planning to set up. Green for things that are setup correctly, and yellow for ones which need attention since they're half-setup.
Before
After
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: