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

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

Closed
wants to merge 11 commits into from

Conversation

0xPrimata
Copy link
Contributor

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.

  • navbar.html add buttons and links to them
  • recommendations.html wraps with div id="recommendations
  • contact.html wraps with div id="contact"
  • navbar.scss properly displays buttons to the navbar

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
Copy link

netlify bot commented Jul 21, 2021

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

🔨 Explore the source changes: bb91cc9

🔍 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
Copy link
Contributor Author

@robzwolf

@0xPrimata 0xPrimata changed the title Navbar + Unfinished README.md Added navbar buttons + Unfinished README.md I was unable to ignore Jul 21, 2021
@louisefindlay23 louisefindlay23 added documentation Improvements or additions to documentation enhancement New feature or request UI labels Jul 21, 2021
@louisefindlay23 louisefindlay23 self-requested a review July 21, 2021 19:40
@louisefindlay23
Copy link
Member

louisefindlay23 commented Jul 21, 2021

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
Copy link
Contributor Author

image
That's fixed on the other branch 🤕

@dtemir
Copy link
Member

dtemir commented Jul 22, 2021

@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
Copy link
Contributor Author

@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.

Copy link
Member

@robzwolf robzwolf left a comment

Choose a reason for hiding this comment

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

I agree with the other comments on this PR – try to fix the white gap and then I'll review this again :)

@0xPrimata
Copy link
Contributor Author

@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
Copy link
Member

@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 robzwolf marked this pull request as draft July 23, 2021 13:39
@robzwolf
Copy link
Member

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
Copy link
Contributor Author

0xPrimata commented Jul 23, 2021

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.

@0xPrimata 0xPrimata closed this Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants