Applies to section #159
Applies to section #159
Conversation
|
Hello and thank you for your contribution! Before we can merge your pull request, we ask that you create a changeset to describe your changes. This will help us manage versions and release notes. You can create a changeset by running |
![translation-platform[bot]](https://avatars.githubusercontent.com/in/7649?s=60&v=4){kind=small}
devisscher
force-pushed
the
applies-to-section
branch
from
b624e0b
to
de532d9
Compare
|
|
||
| return ( | ||
| <Page> | ||
| <AppliesTo |
There was a problem hiding this comment.
I think we could wrap this in a Card so that it stands out a bit more in the stories :)
| }, [productSelector, collectionSelector, eligibility]); | ||
| return ( | ||
| <> | ||
| {productSelector || collectionSelector ? ( |
There was a problem hiding this comment.
This rendering condition is duplicated in the ValueCard component. Probably better if the parent component takes care of the rendering responsibility.
There was a problem hiding this comment.
Reluctant to remove this because the component could be rendered on its own, without the value card. So I wonder if it's better to leave that logic in both places?
| @@ -225,6 +239,16 @@ export function ValueCard({ | |||
| } | |||
| /> | |||
| )} | |||
| {eligibility && | |||
| selectedItems && | |||
| (productSelector || collectionSelector) ? ( | |||
There was a problem hiding this comment.
See my comment in the AppliesTo component.
nit: extract it into a variable for readability
devisscher
force-pushed
the
applies-to-section
branch
from
94ef7d6
to
9d39bdd
Compare
It looks like CurrencyField or Date selectors aren't wrapped in cards, for consistency would it be ok if we left the AppliesTo section outside of a card? (Since it would be meant to be used inside another card, in most cases inside the ValueCard?) |
if that's the pattern we are maintaining currently, sure! |
What problem is this PR solving?
Resolves: #137
Reviewers’ hat-rack 🎩
yarn storybookBefore you deploy