-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(pages): cleanup oauth provider branded buttons #10542
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10542 +/- ##
==========================================
+ Coverage 38.53% 39.76% +1.23%
==========================================
Files 171 172 +1
Lines 27080 27635 +555
Branches 1117 1171 +54
==========================================
+ Hits 10435 10989 +554
- Misses 16645 16646 +1 ☔ View full report in Codecov by Sentry. |
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.
Looking good 🙌🏽
Two small suggestions:
We can make the white box a bit wider to take advantage of the space on the page, and like that the Azure button won't have 2 lines of text (which looks slightly odd) ?
For providers where we don't display an icon, we could display a default icon? 🤔 , maybe the login one from phospor?
@ubbe-xyz that's the "before", the "after" is a bit below in the description above - the all white / dark btns with the brand color only appearing on hover. Also the Azure button.. yeah it is a bit weird. I actually overwrote the name in an application of mine because as you also pointed out, its a bit too long 😅 Maybe we do that here too 🤔 Also a part of the problem with the existing one, was your last comment - those providers actually do have icons, its just their icons are just their brand color so they're totally invisible against a background of the same color 😂 All providers should have a logo imo, we recently shipped a little patch which just looks up the provider logo on our docs page |
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.
Looking good! We can merge and iterate later 🙌🏽
☕️ Reasoning
/signin
page provider btns, using dynamic colors to make themes work in more casesBefore:
Notice that the bottom few providers (
WorkOS
,Twitter
,Twitch
) all do have logos, just they're invisible because they're the same color as the brand color, which is being used as the button background color atm.After:
So this cleans up a good bit:
logoDark
,textDark
variables (were only being used by 1 provider each). Also not a user-facing API, so shouldn't be "breaking", right?bg
/brandColor
providerstyle
option (which most providers have set, I've filled in a few more) and it automatically sets the hover color to that a low opacity version of the brand color with color-mix.color-mix
is in the mdn "baseline 2023" feature set, meaning all browsers do support it now 🙏If yall are cool with this, I can clean up the rest of the providers
style
objects here before merging this 👍After with Hover Brand Colors
🧢 Checklist
🎫 Affected issues
📌 Resources