Skip to content

Separate props for good and bad images#28

Open
ghost wants to merge 13 commits into
mainfrom
Guideline-proposal
Open

Separate props for good and bad images#28
ghost wants to merge 13 commits into
mainfrom
Guideline-proposal

Conversation

@ghost

@ghost ghost commented Mar 22, 2022

Copy link
Copy Markdown

Should we just use a separate prop for good and bad images? A guideline can have more than one image, right?
If you guys think it's good idea, I can go trough and update all the MDX-files 😇

Or should we set up something so that we can have multiple good and bad images? 🤔

@ghost ghost requested review from benja and digitalsadhu March 22, 2022 15:42
Comment thread src/components/Good.js
@@ -2,10 +2,11 @@ import React from 'react';
import classes from '../components/Good.module.css';

export function Good(props) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps Guideline can take an array of images which has the following structure:

{ src: "url-to-image.svg", alt: "alt text" }

Then it'd look something like this:

<Guideline title="Something" images={{ good: [ { src: "url-to-image.svg", alt: "alt text" } ], bad: [ { src: "url-to-image.svg", alt: "alt text" }, { src: "url-to-image.svg", alt: "alt text" } ] }}

Then you can map through these and render them in the component.

Comment thread src/components/Good.js Outdated
import classes from '../components/Good.module.css';

export function Good(props) {
const{className, ...rest} = props

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing formatting here. I would install prettier in your IDE to auto-format things like these. Super convenient!

Comment thread src/components/Guideline.js Outdated
Comment on lines +24 to +29
{goodImage && <Good className="mb-16" >
<img src={`./figma/${goodImage}`} alt="" />
</Good> }
{badImage && <Bad>
<img src={`./figma/${badImage}`} alt="" />
</Bad> }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you would access the prop and do something like props.images.good.map(..) to map through and render the images within the appropriate container. Keep in mind that if you do this, you'd have to update this change for every existing instance of this component.

@benja benja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one comment, otherwise this looks good to me!

Comment thread src/components/Good.js
Comment on lines +8 to +9
// className={`${classes.good} relative border-solid border-0 border-l-4 border-green-500 bg-gray-100 p-5 rounded-r flex align-middle justify-center`}
// {...props}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably can remove this I guess?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant