-
Notifications
You must be signed in to change notification settings - Fork 6
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
[#8546] Style statistics #5750
[#8546] Style statistics #5750
Conversation
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.
Nice job, I left just a few minor notes for you.
|
||
.live_question__progress { | ||
display: flex; | ||
gap: 8px; | ||
margin-bottom: 8px; | ||
position: relative; | ||
} | ||
|
||
.live_question__progress-bar { | ||
background-color: $gray-lighter; | ||
border-radius: 10px; | ||
height: 100%; | ||
position: absolute; | ||
z-index: 0; | ||
} | ||
|
||
.live_question__progress-text { | ||
padding-left: 8px; | ||
padding-right: 8px; | ||
position: relative; | ||
z-index: 1; | ||
} | ||
|
||
.live_question__progress-percentage { | ||
font-weight: bold; |
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.
Can you use live-question
instead of live_question
? Is more BEM-like and already defined like that in the rest of the file.
padding-left: 8px; | ||
padding-right: 8px; |
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.
can you use em
or rem
more, where it makes sense please? I think we prefer that over px, unless its some specific number or similar.
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.
Could you convert this to a functional component as well? Or did you plan to do that in another PR?
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.
This is now converted.
const questionAnsweredTag = django.gettext('Questions Answered') | ||
const categoriesTag = django.gettext('Categories') | ||
const categoriesAnsweredTag = django.gettext('Affiliation Of Answered Questions') |
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.
Could you move these outside the component, if possible? Thanks!
const countPerCategory = this.countCategory(category) | ||
const style = { width: countPerCategory + '%' } | ||
return ( | ||
<div className="live_question__progress" key={index}> |
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 know it's been like that in the code before, but could you change it to use something other than index
?
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.
Thanks, I've now added category_dict
here, as the keys are the category
id
s: https://github.com/liqd/a4-meinberlin/pull/5750/files#diff-2d2b8a5601ea372ea7c39a1e981837692b43614c93d46ab5c0a13f39c8196dd3R54
6937bea
to
d2ab3c0
Compare
@@ -1,13 +0,0 @@ | |||
import StatisticsBox from './StatisticsBox' |
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.
@vellip I noticed that <StatisticsBox />
was also rendered in react_questions_statistics.jsx, but react_questions_statistics.jsx isn't used anywhere or referenced in webpack.common.js, so have removed it.
Thanks for the review, I've now made the changes 👍 |
Describe your changes
This PR styles the statistics box
Tasks