Skip to content

Conversation

@EnumC
Copy link
Member

@EnumC EnumC commented Sep 12, 2017

Static Standalone 404 Page - Would Not Affect Load Time.

@EnumC EnumC added this to the Version 1.0.1 Alcyone Minor milestone Sep 12, 2017
@EnumC EnumC self-assigned this Sep 12, 2017
Copy link
Member

@64bitpandas 64bitpandas left a comment

Choose a reason for hiding this comment

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

Seems like there's a lot of extraneous code here- why do we need FontAwesome for just one GitHub icon?? Why is Roboto in assets? What is "site pages" in 404?

Also, stop squashing your commits, it's better to have lots of smaller commits rather than one or two massive commits.

@EnumC
Copy link
Member Author

EnumC commented Sep 12, 2017

@dbqeo

  1. I will use a cdn for fontawesome too.

  2. Roboto is in assets because it's needed by fonts.css, which main.css have a dependency on.

  3. I added a comment just above "Site Pages". <!--Uncomment Above To Enable Menu-->. It's shown if you uncomment those lines.

  4. Squashing small commits to prevent spam in the commit log. More insight could be found on https://softwareengineering.stackexchange.com/questions/263164/why-squash-git-commits-for-pull-requests

@EnumC EnumC changed the title Remodeled 404 Page Squash Redesigned User-Friendly 404 Page Sep 12, 2017
@EnumC EnumC requested a review from 64bitpandas September 12, 2017 06:39
Copy link
Member

@64bitpandas 64bitpandas left a comment

Choose a reason for hiding this comment

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

Sorry, I wasn't being clear, it was late and i was sleepy :PPP

Anyways, I'll make a few things clearer:

  1. Great 👍
  2. Since Materialize already uses the Roboto font from their server, you can copy that and it won't require any more loading times or file bloating. https://cdnjs.cloudflare.com/ajax/libs/materialize/0.98.2/fonts/roboto/Roboto-Regular.woff2
  3. Sorry I didn't catch that, I was on phone and didn't look at it closely enough. That's great 👍
  4. Squashes like the one you did are fine, it's just that if you are doing lots of things that are unrelated, don't squash them. Several commits to the same file may be squashed. (@EnumC we should write a guide about when to squash in CONTRIBUTING)

Final very important thing:
Angular by default does not actually use a 404 page. I'll make a commit to the branch to make it do so.

TL;DR I'll make sure the 404 redirects properly; everything seems fine except for the duplicate Roboto.

@64bitpandas
Copy link
Member

@EnumC Please verify everything is working and merge when ready. Thanks!

@EnumC EnumC merged commit 81ce30b into develop Sep 14, 2017
@EnumC EnumC deleted the page_404 branch September 14, 2017 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants