Skip to content
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

Add product gallery + basic CSS styling #246

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

castroalves
Copy link

@castroalves castroalves commented Apr 20, 2022

Small improvements based on user feedback

function SingleProduct({product, index}) {
return (
<div style={singleProductStyle}>
<img src={`https://via.placeholder.com/300x200?text=${product.name}`} width={300} height={200} alt={product.name} title={product.name} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using placeholder? The project has images with each product.

Copy link
Author

Choose a reason for hiding this comment

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

The original code didn't have images in it, so I didn't know. 😅 Could you please invite me to the project?

import { gql } from 'graphql-request';

import { graphcms } from '../../lib/_graphcms';

const limit = 1;

const singleProductStyle = {
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to improve styling for all our examples. I'm happy if you want to add these here, but FYI, these may get removed with an updated design when we get some design resources to help here.

Copy link
Author

Choose a reason for hiding this comment

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

That's definitely a thing! Do you already have basic styling from other example that I could use here?

Copy link
Contributor

Choose a reason for hiding this comment

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

these may get removed with an updated design when we get some design resources to help here

function SingleProduct({product}) {
return (
<div style={singleProductStyle} key={product.id}>
<img src={`https://via.placeholder.com/300x200?text=${product.name}`} width={300} height={200} alt={product.name} title={product.name} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Again. Products have images. You can query these 😃

Copy link
Contributor

@notrab notrab left a comment

Choose a reason for hiding this comment

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

Looks good.

I made a comment about the image of the products.

Since we have the product images already, you could query for those.

However, where do you stop with that? These examples were intended to be isolated, and to the point. In this case "using pagination". We already have examples on using Next.js etc that shows how to query/display images.

Personally adding images here is overkill. If we're seeing more people ask for this example then it would make sense, but I don't see a huge demand of people asking for a combined example that they can't put together themselves using the isolated examples.

return (
<div style={singleProductStyle} key={product.id}>
<img src={`https://via.placeholder.com/300x200?text=${product.name}`} width={300} height={200} alt={product.name} title={product.name} />
<img src={image.url} width={200} height={200} alt={product.name} title={product.name} />
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not recommended you use <img /> if you know the width/height but instead use Next.js Image.

However, I would not show an image at all here. We have an example for that.

Copy link
Author

Choose a reason for hiding this comment

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

@notrab we're using Next.js 9 on this project and next/image was introduced in v10. Unless we upgrade it, it's not possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not really relevant though. We should aim to keep these updated regardless.

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

Successfully merging this pull request may close these issues.

2 participants