Card refactoring #76
Card refactoring #76
Conversation
|
✔️ Deploy Preview for helpafamily-margarita-humanitarian ready! 🔨 Explore the source changes: cbc0179 🔍 Inspect the deploy log: https://app.netlify.com/sites/helpafamily-margarita-humanitarian/deploys/6120cd8366766a000846a6ad 😎 Browse the preview: https://deploy-preview-76--helpafamily-margarita-humanitarian.netlify.app |
components/Card.js
Outdated
| addressLabel, | ||
| backgroundImageSource, | ||
| children, | ||
| text, |
There was a problem hiding this comment.
Perhaps text would be better named as description as the use case appears to be to describe the cause?
components/Card.js
Outdated
| </button> | ||
| </div> | ||
| )} | ||
| {children} |
There was a problem hiding this comment.
I'm not sure what your use case of children is expected to be when looking at other cards?
Except maybe to allow additional paragraphs and text objects such as in Health Education Workshop where there are two paragraphs. In which case maybe it is better placed before the action.
There was a problem hiding this comment.
You're right. I forgot about several paragraphs. Will redisign it tomorrow... Somehow ;)
There was a problem hiding this comment.
Sounds like a plan, a case of balancing ease of modification and not doing my usual trick of over-engineering everything haha!
|
@RedFox0x20 What do you think about such component: import Card, { CardAction, CardParagraph, CardTitle } from './Card';
function NewCard() {
return (
<Card backgroundImageSource="/images/HygieneKit.webp">
<CardTitle>{'This is title'}</CardTitle>
<CardParagraph>{'First one paragraph'}</CardParagraph>
<CardParagraph>{'Second one paragraph'}</CardParagraph>
<CardAction onClick={() => {}}>{'Action name'}</CardAction>
</Card>
);
}? |
I like that a lot, very clear what's going on, hides all the specific details ensuring all cards will be the same style. Allows for elements to be in a different order if so desired which is nice. |
|
So far only two cards were reimplemented. If this is approved I will update others. |
|
Thanks so much for your proposal @marekrozmus! I'm in meetings this morning but will review it ASAP today once I have the bandwidth to study it deeply. |
|
I love this proposal. Strong 👍 on going forward. It's elegant, clean, and easy to understand. We can iterate as cards evolve. Thank you @marekrozmus for the proposal and @RedFox0x20 for the review. |
|
@audreyfeldroy all done - please review and merge if OK 🙂 |
| import React from 'react'; | ||
| import { useRouter } from 'next/router'; | ||
|
|
||
| const dollarsToCentsMultiplier = 100; |
There was a problem hiding this comment.
I like how you moved this in here, being Stripe-specific.
| } | ||
| </CardParagraph> | ||
| <CardAction isPending={isPending} onClick={handleSubmit}> | ||
| {`Provide 1 Seat for $${actionCost}`} |
There was a problem hiding this comment.
Nice having a single source for the actionCost.
| router.push(result.url); | ||
| }; | ||
|
|
||
| return [handleSubmit, isPending]; |
There was a problem hiding this comment.
I appreciate how elegantly you factored handleSubmit into here. Tracking isPending like this is also a very nice touch.
|
Merged. Thank you so much @marekrozmus! |
| </a> | ||
| ) : ( | ||
| <button | ||
| className={`btn btn-primary ${isPending ? 'loading' : ''}`} |
There was a problem hiding this comment.
Thanks for your extra eye for detail here, tracking isPending state with the loading indicator. This sort of detail touch really matters and makes the experience better.
Please take a look if this looks OK.
Added also pending state:
All proposals are welcome 🙂
Closes #63