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

Hides the contact icon if empty in config #314

Merged
merged 3 commits into from
Jul 22, 2019
Merged

Hides the contact icon if empty in config #314

merged 3 commits into from
Jul 22, 2019

Conversation

stigrune
Copy link
Contributor

Description

Hi, thank you for this awesome starter!
I made this small change for my own blog and figured it could be useful for others as well, as I’m probably not the only user that don’t need all the available contact options. It’s a simple if condition when the contacts settings is looped. If it’s ‘’(empty) in the config the <li> won’t be returned, thus not showing the icon with a url without the username.
lumen

Copy link
Contributor

@vzhou842 vzhou842 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just a few comments.

@@ -13,7 +13,9 @@ type Props = {
const Contacts = ({ contacts }: Props) => (
<div className={styles['contacts']}>
<ul className={styles['contacts__list']}>
{Object.keys(contacts).map((name) => (
{Object.keys(contacts).map((name) =>
contacts[name] === '' ? ('') :
Copy link
Contributor

@vzhou842 vzhou842 Jul 22, 2019

Choose a reason for hiding this comment

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

I think we should just check if contacts[name] is truthy here - that way, we can catch it if the value is undefined or null, too.

Also, use null instead of an empty string for the empty case. I think React still creates a <span> for an empty string, which we don't want.

This line could be rewritten like: !contacts[name] ? null :

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback :) I fixed this but Prettier changed way more than expected. Sadly, I did not notice before pushing. To avoid messing up the git history I can create a new pull request containing only one commit if wanted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, if you make a new PR just close this one and refer to it from the new one!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code and also the git history in my fork is now fixed. Again thank you for the comments :)

@vzhou842 vzhou842 merged commit b55f228 into alxshelepenok:master Jul 22, 2019
@vzhou842
Copy link
Contributor

Thanks! Made a few edits to satisfy lint.

@codecov
Copy link

codecov bot commented Jul 23, 2019

Codecov Report

Merging #314 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #314   +/-   ##
=======================================
  Coverage   97.22%   97.22%           
=======================================
  Files          35       35           
  Lines         144      144           
  Branches        8        9    +1     
=======================================
  Hits          140      140           
  Misses          4        4
Impacted Files Coverage Δ
src/components/Sidebar/Contacts/Contacts.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa0966b...368217d. Read the comment docs.

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.

2 participants