Skip to content

Conversation

@thehexatown-zaryab
Copy link
Collaborator

No description provided.

module.exports = {
// Add your installed PostCSS plugins here:
plugins: [
// require('autoprefixer'),
Copy link
Owner

Choose a reason for hiding this comment

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

Remove commented lines throughout the code


This project was bootstrapped with [Create React App](https://github.com/facebook/create-react-app).

## Available Scripts
Copy link
Owner

Choose a reason for hiding this comment

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

Don’t remove these until you’ve replaced them all with working components

Copy link
Owner

@tolicodes tolicodes left a comment

Choose a reason for hiding this comment

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

a few minor changes, but looks good

<Container>
<NavWrapper>
<Logo>
<h1>
Copy link
Owner

Choose a reason for hiding this comment

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

the H1 should be part of the logo component. Actually I'm thinking the lgoo should be a png

<SocialMediaLinks>
<a href={"https://linkedin.com"} target={"_blank"} rel={"noreferrer"}>
<Image
src={"/icons/linkedin.svg"}
Copy link
Owner

Choose a reason for hiding this comment

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

strings don't need a {

src={"/icons/github.svg"}
alt={"GitHub"}
width={"22px"}
height={"22px"}
Copy link
Owner

Choose a reason for hiding this comment

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

make a common component SocialIcon with these shared/standard props

</NavWrapper>
<WatchMyStory>
<h1>Watch My Story</h1>
<iframe
Copy link
Owner

Choose a reason for hiding this comment

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

use a youtube react lib so that we can control the video

font-family: "PT Mono", monospace;
text-align: center;

h1 {
Copy link
Owner

Choose a reason for hiding this comment

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

should be sub components

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean I should create separate component for this, instead of nested tag based styling?

width: 820px;
height: 40px;

ul {
Copy link
Owner

Choose a reason for hiding this comment

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

subcomponents

text-align: center;
width: 100%;

h1 {
Copy link
Owner

Choose a reason for hiding this comment

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

sub components

Copy link
Owner

@tolicodes tolicodes left a comment

Choose a reason for hiding this comment

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

Good job!


interface ContainerProps {}
export const Container = styled.div<ContainerProps>`
display: flex;
Copy link
Owner

Choose a reason for hiding this comment

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

We can use Chakra for common components like flex boxes. Goal is to write as little code as possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, got it.

height: 100%;
margin: 0;

li {
Copy link
Owner

Choose a reason for hiding this comment

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

Separate component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please clarify, do you mean I write separate styled component instead of styling nested tags?

Copy link
Owner

Choose a reason for hiding this comment

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

each DOM element (div, li, etc) should be a styled component, so that we can use and see it in isolation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it.

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.

3 participants