-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
Pull Request Test Coverage Report for Build 135
💛 - Coveralls |
frontend/src/ui/Footer.js
Outdated
@@ -6,7 +6,7 @@ import Socials from './Socials' | |||
import { footerHeight, headerBackgroundImage } from './styles/constants' | |||
import { fullWidth } from './styles/mixins' | |||
|
|||
const BaseFooter = glamorous.div(fullWidth, { | |||
const BaseFooter = glamorous.div([fullWidth, 'footer'], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the array necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not... I used it only for the first expect
expect(wrapper.find('div.footer')).toHaveLength(1)
expect(wrapper.find('div div.logo-container')).toHaveLength(1)
expect(wrapper.find('div div.logo-container p')).toHaveLength(1)
expect(wrapper.find('div div.logo-container a.poa-logo')).toHaveLength(1)
expect(wrapper.find('div div.logo-container div.social-container')).toHaveLength(1)
expect(wrapper.find('div div.logo-container div.social-container a.social-item')).toHaveLength(3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not asking why did you add the class. I'm saying that this is the only place where an array is used instead of just listing the arguments. We should use the API consistently even if glamorous accepts different ways of specifying the parameters.
But maybe there was a reason to use it this way here, that was the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I understand the question. Based on what I read about Glomorous, I understood that the way to indicate two or more classes is with an array, and when it is only one class it can be listed or use an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find anything about that in the documentation. Can you give me a link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember where I had read it, or if I just tried and it worked... but I only did it for that test... in any case we can remove it if you think so.
# Conflicts: # frontend/src/ui/BankAccountList.js
Closes #10