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

Week 1 - Portfolio #27

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Bayram89
Copy link

No description provided.

Copy link

@jason-vasilev jason-vasilev left a comment

Choose a reason for hiding this comment

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

Great work, Bayram.

I know it seems like there is a lot to correct, but the reality is that there is one issue repeated many times, that can easily be corrected.

Use class selectors. Give each element a class and style it. This will solve many problems now where you have to do overwrites, or use more specific selectors.

Instead of setting styling on all paragraphs and then overwriting it on each place it varies, make a class for all those variants. Make one class for the .section-paragraph, .footer-paragraph etc. Your styles.css will also become more readable, as when you read the class names you would have a better idea of where and what would they effect.

You are on good track. Just this one tip and you will excel greatly.

Comment on lines +10 to +17
<nav>
<ul>
<li><a href="#about">About</a></li>
<li><a href="#skills">Skills</a></li>
<li><a href="#portfolio">Portfolio</a></li>
<li><a href="#contact">Contact</a></li>
</ul>
</nav>

Choose a reason for hiding this comment

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

Good use of semantic tags and well structured 👏

and organizing highly efficient teams—it’s complex, but that’s what
makes it so exciting.
</p>
<p>📫 Reach out on LinkedIn or email me at bayram9erdem@gmail.com.</p>

Choose a reason for hiding this comment

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

You can make LinkedIn and the email link here.

Suggested change
<p>📫 Reach out on LinkedIn or email me at bayram9erdem@gmail.com.</p>
<p>📫 Reach out on <a hrtef="https://www.linkedin.com/in/bayramerdem" target="_blank">LinkedIn</a> or email me at <a href="mailto:bayram9erdem@gmail.com">bayram9erdem@gmail.com</a>.</p>

Comment on lines +66 to +70
<a
href="https://www.freecodecamp.org/learn/2022/responsive-web-design/build-a-survey-form-project/build-a-survey-form"
target="_blank"
>Survey Form</a
>

Choose a reason for hiding this comment

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

Unfortunately this links to the survey form exercise, but not to your solution. If you would like to upload your solution, you could use CodePen. It's a nifty little platform, where you can register with GitHub to upload small snippets of code. Once you have an account, you can make the so-called pens, and embed them anywhere you link to them.

Comment on lines +73 to +75
<a href="https://food-survey-for-lunch.netlify.app/" target="_blank"
>Food Survey for Lunch</a
>

Choose a reason for hiding this comment

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

Nice to see you've added more projects. Thinking about it, now you have only titles "Food Suvey for Lunch", "Survey Form", "Payment Form". It would help a lot if you would include a screenshot for each and a short paragraph or sentence of description. For example, you can write what technologies were used, or shortly describe the features of the tool.

Comment on lines +91 to +92
<a href="https://github.com/Bayram89" target="_blank">GitHub</a> |
<a href="https://www.linkedin.com/in/bayramerdem/" target="_blank"

Choose a reason for hiding this comment

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

Would be nice if you could use some visuals for the logos here.

display: grid;
grid-template-columns: repeat(auto-fill, minmax(250px, 1fr));
gap: 1.5em;
padding-left: 0;

Choose a reason for hiding this comment

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

This padding-left has already been set to 0 in one place, then you apply global style adding it again, and now you remove it for a second time. This is a lot of overwriting and on a bigger project is could give you headaches to track or you might resolve to bad practices as !important. Have a look at the browser inspector to track such potential issues:
padding

Don't worry too much about it. It will get much better with the time.

background: #D8AE7E;
color: #fff;
padding: 1.5em 0;
text-align: center;

Choose a reason for hiding this comment

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

You already have text-align: center; cascading down from body, but I agree it's better to be set here, than there.

background: #F8C794;
color: white;
text-align: center;
padding: 1.5em;

Choose a reason for hiding this comment

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

I'd recommend you lowering the padding in mobile, as it squeezes the text too much.
big-padding

#portfolio .projects {
display: flex;
flex-direction: column;

Choose a reason for hiding this comment

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

If you add margin: 0 auto; you can center the project boxes on mobile.

padding: 0;
box-sizing: border-box;
}

Choose a reason for hiding this comment

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

Add the following for smoother navigation between sections when clicking the menu links.

html {
  scroll-behavior: smooth;
}

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