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 HTTPS #10

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added HTTPS #10

wants to merge 3 commits into from

Conversation

zahlio
Copy link

@zahlio zahlio commented Oct 24, 2018

Added support for HTTPS.

Added https support
Added docs on how to force https.
@BrandonM25
Copy link

I added the protocol:"https" and it still doesn't work. am I doing something wrong?

index.txt

@mfitzhenry mfitzhenry requested a review from jeserodz October 25, 2018 17:59
Copy link

@jeserodz jeserodz left a comment

Choose a reason for hiding this comment

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

@zahlio thanks for your contribution! I tested this PR with the official Keystone demo project and it's working as expected for HTTP and HTTPS.

I only found a line where we can improve the code quality, you please take a look at the comment?

Thanks!


if (sitemapOptions.protocol == "https") {
sitemapOptions.head = '<urlset xmlns="https://www.sitemaps.org/schemas/sitemap/0.9">';
}

Choose a reason for hiding this comment

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

Would you please align the closing curly brace with the if statement?

@jeserodz
Copy link

@BrandonM25 I just tested it with the official Keystone demo project (https://github.com/keystonejs/keystone-demo) and it's working as expected.

If you're interested in using the forced HTTPS mode, you would need to install this package using git until this PR gets merged, using npm install --save git://github.com/ProjektA/keystone-express-sitemap.git

This is how the setup should look like:

// file: routes/index.js
const sitemap = require('keystone-express-sitemap');

exports = module.exports = function (app) {

  app.get('/sitemap.xml', function (req, res) {
    sitemap.create(keystone, req, res, {
      protocol: 'https'
    });
  });

  // Views
  app.get('/', routes.views.index);
  app.get('/blog/:category?', routes.views.blog);
  app.all('/blog/post/:post', routes.views.post);
  app.get('/gallery', routes.views.gallery);
  app.all('/contact', routes.views.contact);

  // Downloads
  app.get('/download/users', routes.download.users);
}

If this doesn't work, we might be able to help you if you share more details about how are you implementing it in your project.

@BrandonM25
Copy link

I'm at a loss, my project isn't any different from the demo except for adding more pages and styling. here is my index.js its already been minified for web optimization sorry if its messy:

var keystone = require("keystone"), sitemap = require('keystone-express-sitemap'), middleware = require("./middleware"), importRoutes = keystone.importer(__dirname); keystone.pre("routes", middleware.initLocals), keystone.pre("render", middleware.flashMessages); var routes = { views: importRoutes("./views") }; exports = module.exports = function (app) {
app.get('/sitemap.xml', function (req, res) {
sitemap.create(keystone, req, res, { protocol: "https" });
}), app.get("/", routes.views.index), app.get("/blog/:category?", routes.views.blog), app.get("/blog/post/:post", routes.views.post), app.get("/gallery", routes.views.gallery), app.get("/menus/:menucategory?", routes.views.menus), app.get("/menus/menu/:menu", routes.views.menu), app.all("/contact", routes.views.contact), app.get("/cateringfaq", routes.views.cateringfaq)
};`

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.

3 participants