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

uploaded new favicon files #118

Merged
merged 15 commits into from
Mar 8, 2019
Merged

uploaded new favicon files #118

merged 15 commits into from
Mar 8, 2019

Conversation

ashygee
Copy link
Contributor

@ashygee ashygee commented Mar 6, 2019

updating the favicon and touch-icons.

@ashygee ashygee added the status: wip work in progress label Mar 6, 2019
@ashygee ashygee self-assigned this Mar 6, 2019
@ashygee ashygee requested a review from shawnbot March 6, 2019 20:43
"now": "11.4.6",
"now-github-url": "0.1.2",
"serve": "10.0.0"
"serve": "10.1.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

serve kept complaining so I updated. 🤷‍♂️

@@ -0,0 +1,29 @@
/* eslint-disable github/unescaped-html-literal */
Copy link
Contributor

@shawnbot shawnbot Mar 7, 2019

Choose a reason for hiding this comment

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

Our eslint rules don't like unescaped HTML literals, which is definitely good. It's not currently possible to inject unsafe markup into these elements (via process.argv or elsewhere), though, so I think disabling this here is okay.

const meta = ({name, file}) => `<link name="${name}" content="${path(file)}">`

module.exports = [
link({rel: 'icon', type: 'image/png', file: 'favicon.png'}),
Copy link
Contributor

Choose a reason for hiding this comment

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

@ashygee this is currently how we create icon links and meta tags. The link() and meta() functions take an object of HTML attributes, with one exception: file is a filename relative to src/icons, which gets copied at build time (no hot module reloading for now) and prefixed with the path to the file on the server.

So in this case, file: 'favicon.png' causes src/icons/favicon.png to be copied to the output directory (build/icons/favicon.png), and the <link> tag gets href="/icons/favicon.png".

gtag('config', 'UA-126681523-1');
</script>

<link rel='stylesheet' href='https://unpkg.com/@primer/css/dist/layout.css' />
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this is the new unpkg URL for the layout bundle in @primer/css>=12.

@shawnbot
Copy link
Contributor

shawnbot commented Mar 7, 2019

@emplums can I get a gut check on this approach? x0 doesn't bundle the HTML template, so we can't use loaders or plugins to change the static HTML output, which is when we need to inject the octicons. This is a pretty janky way of doing it, but it gets @ashygee unblocked, so I'm inclined to roll with it until we decide to rebuild this site with Blueprints.

src/icons/index.js Outdated Show resolved Hide resolved
Co-Authored-By: ashygee <10384315+ashygee@users.noreply.github.com>
@shawnbot shawnbot removed the status: wip work in progress label Mar 8, 2019
Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

Huzzah, nice work! 🚀

Copy link
Contributor

@emplums emplums left a comment

Choose a reason for hiding this comment

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

@shawnbot this looks ok to me, but I did notice that the favicon only renders once the browser is rehydrated, so you see a missing favicon when it first loads and you don't see the correct favicon until later :/

@ashygee ashygee merged commit 6f0959b into master Mar 8, 2019
@ashygee
Copy link
Contributor Author

ashygee commented Mar 8, 2019

This closes #114

@shawnbot
Copy link
Contributor

shawnbot commented Mar 8, 2019

@emplums Hmm that's weird, because it should be completely static. And it looks like it is if you visit view-source:https://primer-style-favicon-update.now.sh/ — I see the following minified HTML:

<!DOCTYPE html><head><title>Primer open-source projects</title><meta charset="utf-8"><meta name="viewport" content="width=device-width,initial-scale=1"><link rel="icon" type="image/png" href="/icons/favicon-32.png"><link rel="icon" type="image/png" href="/icons/favicon-48.png"><link rel="icon" type="image/png" href="/icons/favicon-96.png"><link rel="icon" type="image/png" href="/icons/favicon-144.png"><link rel="icon" type="image/png" href="/icons/favicon-192.png"><link rel="apple-touch-icon" type="image/png" href="/icons/touch-icon-iphone.png"><link rel="apple-touch-icon" type="image/png" href="/icons/touch-icon-ipad.png"><link rel="apple-touch-icon" type="image/png" href="/icons/touch-icon-ipad-retina.png"><link rel="apple-touch-icon" type="image/png" href="/icons/touch-icon-iphone-retina.png"><link rel="icon" type="image/png" href="/icons/android-icon.png"><link name="msapplication-square310x310logo" content="/icons/microsoft-icon.png">

So maybe it's just your cache warming up and not an SSR/hydration issue?

@shawnbot shawnbot deleted the favicon-update branch March 8, 2019 23:38
@shawnbot shawnbot mentioned this pull request Mar 8, 2019
dkngyn pushed a commit to dkngyn/primer.style that referenced this pull request Jun 9, 2019
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.

None yet

3 participants