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

refactor(sns): simplify conditional display #119

Merged
merged 1 commit into from
Jan 12, 2017
Merged

Conversation

pidupuis
Copy link
Collaborator

I replaced every:

<% if(theme.sns.twitter == undefined){ %>
<% } else{ %>
...
<% } %>

by a simpler and more readable:

<% if(theme.sns.twitter){ %>
...
<% } %>

@Halyul
Copy link
Contributor

Halyul commented Jan 12, 2017

I think this is unnecessary, because not every has his/her Twitter, Facebook account.
If you change this, the sns icons will always show on the footer. ( Just in my mind, hasnt tested.

@pidupuis
Copy link
Collaborator Author

I've tested it and it works the same because theme.sns.twitter returns false if nothing is set in the config for twitter.

I agree it does not work exactly the same because I exclude every falsy value, not only undefined.

Using

<% if(theme.sns.twitter == undefined){ %> 
<% } else { %>
... 
<% } %> 

is exactly the same as:

<% if(theme.sns.twitter != undefined){ %> 
... 
<% } %> 

which is already better.

It is even better to use the strict equality:

<% if(theme.sns.twitter !== undefined){ %> 
... 
<% } %> 

But with every one of these codes, a config containing:

facebook: "https://facebook.com/foo/bar"
twitter: false
linkedin:

would result by showing both facebook and twitter icons.

With my version, only the facebook icon would show up.

@Halyul
Copy link
Contributor

Halyul commented Jan 12, 2017

OK, got it.

@iblh iblh merged commit a76287d into canary Jan 12, 2017
@pidupuis pidupuis deleted the refactor/sns-display branch January 12, 2017 17:23
@iblh iblh mentioned this pull request Jan 12, 2017
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