adding rectangle shape and twitter logo in top right #10
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Hey there 👋 , I requested some changes, can you do those before we continue the review? Please also reference original issues for tracking purposes.
Additionally, I've noticed that the feature is missing, as seen in this screenshot:
As per original issues, we are missing:
- A switch that will toggle if the logo is shown or not
- An option to select aspect ratio (1:1, 16:9 etc.)
components/Main.jsx
Outdated
| { | ||
| <Box m='0 auto'> | ||
| <Box className='con' style={{background : bg}} minW={pic_size} maxW={pic_size} rounded="sm" px={padX} py={padY} ref={tweetRef}> | ||
| <Stack direction={'row'} > |
There was a problem hiding this comment.
What's the point of this <Stack /> here? You only have 1 child here
components/Tweet.jsx
Outdated
| @@ -2,7 +2,7 @@ | |||
| import {format} from 'date-fns' | |||
| import { Box, Text, Image } from "@chakra-ui/react" | |||
|
|
|||
| const Tweet = ({tweet, showTime, showMetrics, showSource, showImage}) => { | |||
| const Tweet = ({tweet, showTime, showMetrics, showSource}) => { | |||
There was a problem hiding this comment.
Changes in this file, as seen here seem to be out-of-scope
styles/main.css
Outdated
| white-space: pre-line; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| align-content: 'center' ; |
public/main.css
Outdated
| @@ -47,11 +46,10 @@ footer { | |||
| white-space: pre-line; | |||
| display: flex; | |||
| justify-content: center; | |||
| align-items: center; | |||
| align-items: 'center'; | |||
There was a problem hiding this comment.
This is invalid css. The original is correct:
| align-items: 'center'; | |
| align-items: center; |
| setTweetData(null) | ||
| } | ||
| import Head from "next/head"; | ||
| import { useState, useRef } from "react"; |
There was a problem hiding this comment.
There are too many changes in this file that are a result of running a prettier. Because this project is not using prettier, please disable it, and make your changes again before we can continue review process
There was a problem hiding this comment.
Hey @mannol, since I built this project a long back (when I was still new to programming), it doesnt follow the industry best practices. Do you think I should update the main branch with the best practices like prettier, maybe eslint, typescript? Will that be helpful?
There was a problem hiding this comment.
The toggle icon seems to be working properly, although, when I toggle it it seems to affect the text layout. We should make it such that once toggled it doesn't affect the text (see images).
No icon:
With icon (the word "da" gets shifted onto the next line):
Also, it would appear that when selecting 1:1 or 16:9 ratio, the outer box changes while the inner box stays the same. We need to make it so that the inner box (the one with the tweet) respects the selected ratio (see sketch for more details)
|
Hey @huskice, thanks for the PR! Nice idea. Please allow me some time to review this. |
Solves issue #6