search

MLH-Fellowship / pod-3.1.3-portfolio

Portfolio website template for Pod 3.1.3, Practically Pi!
https://practically-pi-portfolio-template.netlify.app/
MIT License
10 stars 13 forks source link

Added navbar buttons + Unfinished README.md I was unable to ignore #52

Closed 0xPrimata closed 3 years ago

0xPrimata commented 3 years ago

Why This PR Adds Value

Wanted to solve issue #50 Add Recommendations and Contact Me buttons to navbar.

What This PR Adds

Adds Recommendations and Contact Me to navbar and div ids on recommendations.html and contact.html. It also adds an unfinished README.md file which I already had locally.

Screenshot

image

How This PR Could Be Improved

css display review. #48 might have an issue #48 here if it edits how navbar displays things.

Issue This PR Closes

This closes issue #50

netlify[bot] commented 3 years ago

✔️ Deploy Preview for practically-pi-portfolio-template ready!

🔨 Explore the source changes: bb91cc9dbb217837f84b840350039a4a744d1a2c

🔍 Inspect the deploy log: https://app.netlify.com/sites/practically-pi-portfolio-template/deploys/60f99010bd107e00080872cd

😎 Browse the preview: https://deploy-preview-52--practically-pi-portfolio-template.netlify.app/

0xPrimata commented 3 years ago

@robzwolf

louisefindlay23 commented 3 years ago

Hi, @SnowPrimate. Just responding to your Discord PM, requesting a review.

Looks great. Might be nice to add screenshots of the website and a link to the deployed web app for easy access.

The rendered MD for the directory structure looks a little strange. Not sure if it's supposed to look like this?

Directory Structure Readme

0xPrimata commented 3 years ago

image That's fixed on the other branch 🤕

dtemir commented 3 years ago

@SnowPrimate having a little issue here when I put the browser to half of my screen. Probably caused by the fact the logo is set to the center at all times. Issue on the website. Navbar links in the right do not move when screen sized differently

0xPrimata commented 3 years ago

@dtemir Shoot, that would also be an issue despite my modifications. I would say let's use justify-content: space-between? I'll fix that tomorrow as soon as I can, for sure before our meeting! Really late around here.

0xPrimata commented 3 years ago

@robzwolf I've discussed with @jagnani73 and he said that he and @dtemir are working on the white gap issue since they are working on the navbar's CSS. I'll wait for their solution and fix it from there if there is anything to be done. How should I proceed here? should I close this pull request and then PR again after their PR gets merged?

robzwolf commented 3 years ago

@robzwolf I've discussed with @jagnani73 and he said that he and @dtemir are working on the white gap issue since they are working on the navbar's CSS. I'll wait for their solution and fix it from there if there is anything to be done. How should I proceed here? should I close this pull request and then PR again after their PR gets merged?

There are a few acceptable approaches here. I think you should mark this pull request as a Draft one (see this guide) and then convert it to a full PR once you're ready for review.

robzwolf commented 3 years ago

Hey @SnowPrimate – I've marked this PR as a Draft for you.

Can you take a look at the latest version of main and work out which changes are still needed? I've a feeling a lot of this PR is perhaps no longer necessary.

0xPrimata commented 3 years ago

Seems like the navbar has been rebuilt, including my changes, but it has a few issues. I'll close this issue and open a new ticket to deal with those specifically.