-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature/pfe 3 payex badge #928
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #928 +/- ##
========================================
Coverage 72.30% 72.30%
========================================
Files 212 212
Lines 4586 4586
Branches 1306 1306
========================================
Hits 3316 3316
Misses 1126 1126
Partials 144 144
Continue to review full report in Codecov by Sentry.
|
Size Change: +60 B (0%) Total Size: 1.12 MB ℹ️ View Unchanged
|
@WMeander I love it !!! 🌟 awesome stuff !!! 🚀 Indeed the PayEx side of things tends to be forgotten since our designers are 100% SwedbankPay 🙈. So the issue you've spotted (and fixed 🌟 ) for badges is likely in other components too. Awesome that you could open the PR. This is the way to go. I'm the only dev working on the design system, so you can imagine I have way too much to carry to do all the work that deserves to be done. Open Sourcing the work via PRs (like you just did) is 100% the way to go 💪 I have a big PR in review right now (we're migrating icons), which will probably get approved and merged this week. It's touching a huge amount of files, so I don't want to tempt the GH-Merge-Conflicts gods, I'll wait for it and I'll merge this one right after 😅 Question for you: I'm not super clear on which projects are using the PayEx side of the DS. Do you know? Are we sure the other PayEx projects consuming the DS library are fine with those UI changes? (And please, bring some more of those PR 👍 it's needed ⭐ let me know if you have questions, doubts, ideas, etc. Also, so you know, I'm trying to migrate little by little from LESS to pure CSS where possible. Like switching to CSS custom properties for example (although it wasn't really ready for it there yet). I'm saying it for the future, in case you're planning to open more PRs, which I hope 🤞 😃 ) |
@goldenraphti Thanks! Yes, we do have other components with the same issue where colors are out of sync. We're planning to update some of them the upcoming sprint. Ah, I'll wait for that big PR to be done. (The payex designer is also reviewing the changes in this feature branch so we can wait) Most of PayEx web:s are using the old versions of the design guide (v2). I think its around version 7 that PayEx design guide had a bigger update and it's not that many system using the newer versions. This user story comes from payex designer so i think we're good with the changes done. (All PayEx user stories coming up are also from the payex designer, so should be good to go) |
awesome @WMeander
|
@goldenraphti The designer has approved the new colors for badge component. So this branch is ready to be merged whenever you're done with your icon review |
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'll merge it once SWED-2276 gets approved and merged (hopefully today 😉) 👍
Description
Updated colors for PayEx Badge compontent
Motivation and Context
Colors for PayEx bagdes did not follow the current color scheme and needed to be updated. Also accessibility contrast score low on some badges.
Also added variables used by swedbank pay to payex variables wich is causing compiler error due to missing variables when using payex design guide on clients.
How Has This Been Tested?
Ran snapshot test for current component, style linting for less files. Visual review sent to designer.
Fixed linting for most files belonging to payex.
Screenshots (if appropriate):
Before:
After:
Types of changes
Checklist:
Review instructions
Review instructions