Skip to content

Conversation

@kateharwood
Copy link
Contributor

@kateharwood kateharwood commented Nov 12, 2020

First pass at navbar.

@Letsgohyde still need to add in the real icons (and definitely need a better version of the logo, I'm just using the favicon currently)

Also, does not play well with mobile / is not responsive yet. What are our plans regarding mobile and responsiveness for this iteration? Do we have separate designs?

Screen Shot 2020-11-12 at 2 11 51 PM

@netlify
Copy link

netlify bot commented Nov 12, 2020

Deploy preview for cmu-delphi-main ready!

Built with commit 255fc1e

https://deploy-preview-26--cmu-delphi-main.netlify.app

@kateharwood kateharwood changed the title Navbar Navbar / Footer Nov 13, 2020
@kateharwood
Copy link
Contributor Author

Added footer

Screen Shot 2020-11-13 at 2 32 20 PM

@kateharwood kateharwood changed the title Navbar / Footer Header / Footer Nov 13, 2020
@kateharwood
Copy link
Contributor Author

Header updated:
Screen Shot 2020-11-13 at 4 49 53 PM

@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 384 512"><!-- Font Awesome Free 5.15.1 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL 1.1, Code: MIT License) --><path d="M384 144c0-44.2-35.8-80-80-80s-80 35.8-80 80c0 36.4 24.3 67.1 57.5 76.8-.6 16.1-4.2 28.5-11 36.9-15.4 19.2-49.3 22.4-85.2 25.7-28.2 2.6-57.4 5.4-81.3 16.9v-144c32.5-10.2 56-40.5 56-76.3 0-44.2-35.8-80-80-80S0 35.8 0 80c0 35.8 23.5 66.1 56 76.3v199.3C23.5 365.9 0 396.2 0 432c0 44.2 35.8 80 80 80s80-35.8 80-80c0-34-21.2-63.1-51.2-74.6 3.1-5.2 7.8-9.8 14.9-13.4 16.2-8.2 40.4-10.4 66.1-12.8 42.2-3.9 90-8.4 118.2-43.4 14-17.4 21.1-39.8 21.6-67.9 31.6-10.8 54.4-40.7 54.4-75.9zM80 64c8.8 0 16 7.2 16 16s-7.2 16-16 16-16-7.2-16-16 7.2-16 16-16zm0 384c-8.8 0-16-7.2-16-16s7.2-16 16-16 16 7.2 16 16-7.2 16-16 16zm224-320c8.8 0 16 7.2 16 16s-7.2 16-16 16-16-7.2-16-16 7.2-16 16-16z"/></svg> No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of checking these in, can you pull them down from npm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's handle this in another PR.

Copy link
Contributor

@tildechris tildechris left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Kate!

@kateharwood kateharwood marked this pull request as ready for review November 17, 2020 19:01
Copy link
Member

@sgratzl sgratzl left a comment

Choose a reason for hiding this comment

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

can you resolve the conflicts, please

Moreover, can you use relative links otherwise it won't work on our test environment

config.toml Outdated
name = "Delphi"
identifier = "about"
name = "About"
url = "https://delphi.cmu.edu/"
Copy link
Member

Choose a reason for hiding this comment

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

please use a relative link here /about isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'm not sure if matching the active url to these urls to highlight active tab will work if these are changed to be relative, might have to rethink that then if it's important to make these relative.

config.toml Outdated
[[menu.main]]
name = "COVIDcast"
pre = "map"
url = "https://covidcast.cmu.edu"
Copy link
Member

Choose a reason for hiding this comment

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

will soon be /covidcast

config.toml Outdated
[[menu.main]]
name = "Blog"
pre = "blog"
url = "https://delphi.cmu.edu/blog"
Copy link
Member

Choose a reason for hiding this comment

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

same here /blog

@@ -0,0 +1 @@
<a class="delphi-text-logo" href="https://delphi.cmu.edu/">DELPHI</a> No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

see https://gohugo.io/functions/relref/ to create references in hugo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you use this to link to the homepage from anywhere on the site? Relref seems to be relative to where the user is on the site? Definitely not understanding something here.

<table class="uk-table footer-table">
<thead>
<tr>
<th class="header">
Copy link
Member

Choose a reason for hiding this comment

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

should we prefix css classnames by their area?, e.g. footer-header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

<link rel="preconnect" href="https://fonts.gstatic.com">
<link href="https://fonts.googleapis.com/css2?family=Open+Sans:wght@300&display=swap" rel="stylesheet">
<link href="https://fonts.googleapis.com/css2?family=Open+Sans:wght@600&display=swap" rel="stylesheet">
<link href="https://fonts.googleapis.com/css2?family=Open+Sans:wght@700&display=swap" rel="stylesheet">
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to load all fonts at one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

</li>
{{ $currentPage := . }}
{{ range .Site.Menus.main.ByWeight }}
<li class="{{ if (eq $currentPage.URL .URL) }}active{{ end }}">
Copy link
Member

Choose a reason for hiding this comment

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

have you tried using https://gohugo.io/functions/hasmenucurrent/? instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that wasn't working for me, and I looked at some other posts and saw that it seems not to work without putting the menu items in the "Front Matter" for the pages, and I saw this used as a workaround

@tildechris tildechris self-requested a review November 17, 2020 21:51
Copy link
Contributor

@tildechris tildechris left a comment

Choose a reason for hiding this comment

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

LGTM.

@kateharwood kateharwood merged commit a734a27 into dev Nov 17, 2020
@sgratzl sgratzl deleted the header branch November 23, 2020 19:54
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.

4 participants