-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: carousel to grid refactor #9
Conversation
src/skills-builder/skills-builder-modal/view-results/ProductTypeBanner.jsx
Outdated
Show resolved
Hide resolved
src/skills-builder/skills-builder-modal/view-results/ProductCardGrid.jsx
Outdated
Show resolved
Hide resolved
}, | ||
})); | ||
|
||
expect('"Prospector" degrees').toBeTruthy(); |
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.
Am I missing something about how this works? Aren't strings always going to be truthy?
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.
Good question! So the strings for degrees, programs, executive education, and boot camps will not appear by default. They will only appear if those values appear in the query string. So this is testing that the UI is reading properly from the query string. We have separate tests for the hook that extracts these values from the query string. This is just to make sure something renders to the page that matches that product type.
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 does expect()
automatically search through the context for a string? If you put any random string as the argument to expect, wouldn't it be truthy? I think you need to look for the string in the screen object.
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.
Oh yes! I didn't check the screen
. I see what you mean now 👍 I'll fix this
Codecov Report
@@ Coverage Diff @@
## main #9 +/- ##
=======================================
Coverage ? 91.61%
=======================================
Files ? 30
Lines ? 322
Branches ? 45
=======================================
Hits ? 295
Misses ? 27
Partials ? 0 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
1f19e61
to
e11fd21
Compare
e11fd21
to
b070d8a
Compare
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.
This looks good. Thanks for all the fixes!
This PR will refactor the Product recommendations carousel to be a
CardGrid
that has an option to expand. The component formerly known asCarouselStack
has been removed completely and replaced with theRecommendationStack
. In addition, there have been two new components added:ProductTypeBanner
andProductCardGrid
, which live insideRecommendationStack
and help to break things up/make them easier to grok.JIRA
APER-2483
Screenshots
Large screens
Medium screens
Small/X-small screens