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.

Sign up for GitHub

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

Jump to bottom

Move front page from WordPress into app #2740

Merged
merged 20 commits into from
Nov 27, 2015
Merged

Move front page from WordPress into app #2740

merged 20 commits into from
Nov 27, 2015

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Nov 25, 2015

  • Deal with main.js, probably just figure out exactly what js stuff is actually used on the front page and re-write it
  • Put a rel="feed" link for the blog feed in the <head>
  • Make the login etc links in the top-right work
  • Google Analytics?
  • Integrate home.html.jinja2 with base.html.jinja2

@seanh seanh added the WIP label Nov 25, 2015
@seanh seanh self-assigned this Nov 25, 2015
@nickstenning nickstenning mentioned this pull request Nov 25, 2015
Merged
@seanh
Copy link
Contributor Author

seanh commented Nov 26, 2015

@robertknight Want to review this?

The minified CSS file needs to be recreated as a nice, minimal one but I suggest doing that as a separate PR. When we do that we can also migrate the front_page_js and front_page_css bundles into the site one (or maybe some stuff will remain that's only needed on the front page).

In the meantime I think we can go ahead and publish this front page as-is if we need to?

I'm getting a couple of errors in the console that it's failing to load some fonts, but I'm also seeing these in production, I'm guessing it's from the minified CSS file and will disappear when we sort that out.

@seanh seanh removed their assignment Nov 26, 2015
@seanh seanh removed the WIP label Nov 26, 2015
nickstenning
nickstenning reviewed Nov 26, 2015
View reviewed changes
h/templates/home.html.jinja2
<div class="container">
<span>
We're hiring
<a href="{{ base_url }}jobs/#developer">developers</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good reason to have all these links as {{ base_url }}...? Can't they just be relative links?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It {{ base_url }} isn't the root URL of a domain? But not sure if we care about that

@nickstenning
Copy link
Contributor

Not sure about copying the compiled h/static/styles/front-page/main.css into this repository. This is where it's built from, but I highly doubt we need all of that for just the front page. Even if we do, we should probably be using bootstrap_css as already provided rather than adding another copy of bootstrap to the repository.

@seanh
Copy link
Contributor Author

seanh commented Nov 26, 2015

Not sure about copying the compiled h/static/styles/front-page/main.css into this repository. This is
where it's built from, but I highly doubt we need all of that for just the front page. Even if we do, we
should probably be using bootstrap_css as already provided rather than adding another copy of
bootstrap to the repository.

Yes, my suggestion was that we could fix that in a separate pr though, possibly at the same time as implementing Conor's new design, and possibly to be done by Robert instead of me

@seanh
Copy link
Contributor Author

seanh commented Nov 26, 2015

I think this is just waiting to hear from Robert about Apple icons now

@seanh
Move the hypothes.is front page into the app
a587f4f 
Initial move of the hypothes.is front page into the app, much tidying
up and migrating of resources stil to do.

For this commit I did a view-source on https://hypothes.is/ and
copy-pasted it into home.html.jinja2, then:

- Routed "/" to home.html.jinja2, replaces the previous "/" route that went to
  the help page
- Reformatted the HTML to make it more manageable
- Removed the link to the empty RSS comments feed
- Removed tags in the <head> to do with installing the site as a web app on
  iOS, Android and Windows Mobile
- Removed WordPress RSD EditURI metadata tags
- Removed wlwmanifest.xml metadata link
- Removed the <link rel="shortlink" whose value was the same as the
  canonical link
- Removed the Google Analytics JavaScript
- Removed the "You need to update your browser" message and browsehappy.com
  link that was shown to IE < 9 users
- Removed the broken <script> link to showhighlights.js
- Removed inline JS and CSS to do with "WordPress emojis"

This "works" but still pulls all of the resources (CSS, images, js...) from the
production WordPress instance. Much tidying up still to do.
@seanh
Fix broken /fund link
d854778
@seanh
Copy Font Awesome from WordPress into app
87efd26 
This uses a completely separate front_css webassets bundle for now, the
CSS for the front page should be integrated with that in the site_css
bundle later.
@seanh
Remove wp-syntax CSS link
777ccc4 
This is a syntax-highlighting plugin for WordPress
@seanh
Copy the WordPress theme CSS into the app
fd1af62 
This just copies the already compiled and minified main.css file into
the app, doesn't yet try to copy the source Sass files and recreate the
assets pipeline for them.
@seanh
Use jquery from webassets not WordPress
62d1ce8 
Also move the <script> tag from the <head> to the bottom of the <body>.

This also removes the jquery-migrate.min.js that was previously included
as it doesn't appear to be needed.
@seanh
Use {{ base_url }}
f76d93b 
Instead of hardcoding http(s)://hypothes.is/.
@seanh
Copy apple-touch-icons from WordPress
c83adb8 
Also reduce these to just the one 152x152 icon instead of several
different ones. This should be sufficient. The tiny downside is that
older devices download the larger image and then rescale it.
@seanh
Copy favicons from WordPress
0fb71c5
@seanh
Copy Chrome icon from WordPress
6c65a6a
@seanh
Remove modernizr from front page
a8ef66f 
Not used
@seanh
Remove main.js
b8521c1 
The remove the <script> link to the main.js file of the WordPress instance.
Replace this with some inline JavaScript that does the one thing that (as far
as I know) main.js was being used for on the front page: to show the
install-chrome-extension button in Chrome but show the install-bookmarklet
button in all other browsers.

Changing this to a tiny bit of inline js also means that the install-chrome-
extension button is visible immediately in Chrome, instead of seeing the
bookmarklet button for a second then seeing it change.
@seanh
Add a <link rel="alternate" for the RSS feed
956d7cd
@seanh
Fix the account links on the front page
6a17181 
Fix the Sign in/Sign out/Create an account links in the top right of the
front page.

When logged in you see:

* Your username, links to your My Annotations page
* Sign out, redirects to front page after logout

When not logged in you see:

* Sign in, links to /login and redirects to /stream after login
* Create an account, links to /register and redirects to front page

Remove the "You have logged out" flash message. Since these flash messages are
only shown in the Angular app, and logging out redirects you to the front page,
the message is not shown when you log out but the next time you visit for
example /stream, by which time you may well be logged in again.

Note that this create an account behavior is badly broken: You are not logged
in after creating an account, instead it sends you an activation email, but
you aren't shown any message to say that an activation email has been sent.
You're just redirected to the logged-out front page and told nothing. Clicking
on the link in the activation email also just redirects you to the logged-out
front page without logging you in or telling you that you've activated, but it
also sets multiple flash messages ("Please check your email to activate" and
"You have activated" which are all shown at once the next time the Angular app
loads (e.g. when you visit /stream).

This, however, is all the same as the current behaviour in production (!). This
needs to be fixed in a separate commit.
@seanh
Add Google Analytics to front page
5a5c085 
But this should be integrated with base.html.jinja2, instead of
duplicating.
@seanh
HTML validator fixes to front page
9b45c7a 
Various fixes to the front page HTML, suggested by https://validator.w3.org/
@seanh
Fix modal dialogs on front page
5945918 
Bootstrap's JavaScript wasn't being included.
@seanh
Integrate home.html.jinja2 with base.html.jinja2
6ae7f76 
Make home.html.jinja2 extend base.html.jinja2, as other site pages do.

This removes duplication of, for example, the Google Analytics tracking code
which should be on every page so is in base.html.jinja2.

The <html lang="en" and the various favicons and apple-touch-icons etc move
into base.html.jinja2, so now every page has them.
@seanh
Remove unused code from help.html.jinja2
fba2072 
The help route is no longer served at "/" so is_index is never true.
@seanh
Fix broken bookmarklet link
ffb9032 
/embed.js not /app/embed.js, and get it from request.route_url() instead
of hardcoding it.
robertknight added a commit that referenced this pull request Nov 27, 2015
@robertknight
Merge pull request #2740 from hypothesis/trello-178-new-homepage
a1ba8dc 
Move front page from WordPress into app
@robertknight robertknight merged commit a1ba8dc into master Nov 27, 2015
@robertknight robertknight deleted the trello-178-new-homepage branch November 27, 2015 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@seanh @nickstenning @robertknight