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

Mino feat explore thumbnail #46

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Conversation

kakizaki55
Copy link
Contributor

@kakizaki55 kakizaki55 commented Apr 28, 2022

I got the avatar to render correctly on the font-end. but this pull request must be made in tandem with the back end.
---> look at the backend PR for more details.
Screen Shot 2022-04-28 at 12 38 11 PM

@netlify
Copy link

netlify bot commented Apr 28, 2022

Deploy Preview for geo-tone-staging ready!

Name Link
🔨 Latest commit 26eca35
🔍 Latest deploy log https://app.netlify.com/sites/geo-tone-staging/deploys/626b1d84f8a0440008066211
😎 Deploy Preview https://deploy-preview-46--geo-tone-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@jlaurentpdx jlaurentpdx left a comment

Choose a reason for hiding this comment

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

Awesome work, Minoka! I added some comments for appreciation and requested changes - summary here:

  • App.jsx appears to have been duplicated into the root folder from src; this should get removed.
  • The avatars rendered on the Explore page currently read "image" by screen readers; let's add an empty alt tag to have that bypassed.

@@ -33,6 +33,7 @@ export default function Explore() {
key={`project.title${index}`}
onClick={() => handleExploreProject(project.projectId)}
>
<img className={styles.thumbnail} src={project.avatar} />
Copy link
Contributor

Choose a reason for hiding this comment

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

We should include an empty alt tag here (alt="") to prevent the image from being read by screen readers.

@@ -0,0 +1,73 @@
import { BrowserRouter as Router, Routes, Route } from 'react-router-dom';
Copy link
Contributor

Choose a reason for hiding this comment

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

App.jsx should exist in the src folder, but it appears that it may have accidentally been duplicated into the root folder here.

src/utils/imgUtils.js Show resolved Hide resolved
src/views/Explore/Explore.css Show resolved Hide resolved
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.

2 participants