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

Migrate asset and authors helpers to es6 #10611

Merged
merged 2 commits into from Mar 26, 2019
Merged

Migrate asset and authors helpers to es6 #10611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14854c8
update asset helper to use newer code standards
vikaspotluri123 Mar 13, 2019
c8dcaa2
update authors helper to use newer code standards
vikaspotluri123 Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions core/server/helpers/asset.js
View file
Open in desktop
@@ -1,11 +1,11 @@
// # Asset helper
// Usage: `{{asset "css/screen.css"}}`, `{{asset "css/screen.css" ghost="true"}}`
// Usage: `{{asset "css/screen.css"}}`
//
// Returns the path to the specified asset. The ghost flag outputs the asset path for the Ghost admin
const proxy = require('./proxy'),
get = require('lodash/get'),
getAssetUrl = proxy.metaData.getAssetUrl,
SafeString = proxy.SafeString;
// Returns the path to the specified asset.
const proxy = require('./proxy');
const get = require('lodash/get');
const {SafeString} = proxy;
const {getAssetUrl} = proxy.metaData;

module.exports = function asset(path, options) {
const hasMinFile = get(options, 'hash.hasMinFile');
Expand Down
44 changes: 24 additions & 20 deletions core/server/helpers/authors.js
View file
Open in desktop
Expand Up @@ -6,27 +6,31 @@
// By default, authors are separated by commas.
//
// Note that the standard {{#each authors}} implementation is unaffected by this helper.
const proxy = require('./proxy'),
_ = require('lodash'),
urlService = require('../services/url'),
SafeString = proxy.SafeString,
templates = proxy.templates,
models = proxy.models;

module.exports = function authors(options) {
options = options || {};
options.hash = options.hash || {};
const proxy = require('./proxy');
const _ = require('lodash');
const urlService = require('../services/url');
const {SafeString, templates, models} = proxy;

const autolink = !(_.isString(options.hash.autolink) && options.hash.autolink === 'false'),
separator = _.isString(options.hash.separator) ? options.hash.separator : ', ',
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not fully sure if we can get rid of this logic, but it seems like we should be able to 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔Thinking if losing the isString check in the destructuring default value here has any downsides. Seems it differs only when non-string value is passed to separator, which should be wrong usage so can be ignored as case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the only possible case I can think of is if the theme author passes in a variable (e.g. number of posts) 😟

Copy link
Contributor

Choose a reason for hiding this comment

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

@vikaspotluri123 Ah yeah, but passing number of posts in as a separator is still wrong usage in a way right? 😄I mean previously we would have just ignored it and set , as separator, so I think the changes are still ok ;) Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree 😄

prefix = _.isString(options.hash.prefix) ? options.hash.prefix : '',
suffix = _.isString(options.hash.suffix) ? options.hash.suffix : '',
limit = options.hash.limit ? parseInt(options.hash.limit, 10) : undefined,
visibilityArr = models.Base.Model.parseVisibilityString(options.hash.visibility);
module.exports = function authors(options = {}) {
options.hash = options.hash || {};
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't use nested destructuring here because the input can be considered untrusted


let output = '',
from = options.hash.from ? parseInt(options.hash.from, 10) : 1,
to = options.hash.to ? parseInt(options.hash.to, 10) : undefined;
let {
autolink,
separator = ', ',
prefix = '',
suffix = '',
limit,
visibility,
from = 1,
to
} = options.hash;
let output = '';
const visibilityArr = models.Base.Model.parseVisibilityString(visibility);

autolink = !(_.isString(autolink) && autolink === 'false');
limit = limit ? parseInt(limit, 10) : limit;
from = from ? parseInt(from, 10) : from;
to = to ? parseInt(to, 10) : to;

function createAuthorsList(authors) {
function processAuthor(author) {
Expand All @@ -36,7 +40,7 @@ module.exports = function authors(options) {
}) : _.escape(author.name);
}

return models.Base.Model.filterByVisibility(authors, visibilityArr, !!options.hash.visibility, processAuthor);
return models.Base.Model.filterByVisibility(authors, visibilityArr, !!visibility, processAuthor);
}

if (this.authors && this.authors.length) {
Expand Down