-
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
apps/item detail templates: use label/badges from model property and … #4585
apps/item detail templates: use label/badges from model property and … #4585
Conversation
@CarolingerSeilchenspringer That was a long text and a lot of questions yesterday, but the only question actually is: Can we keep the "Kosten: keine Angabe" when the budget is 0 on the lists for Bürgerhaushalt and Kiezkasse or shouldn't there be any badge in that case? |
@fuzzylogic2000 checked and find all three solutions you mention good. Have it the same everywhere sounds the best option. |
5abd4c5
to
be739f4
Compare
…move badge properties to mixin to use in topics
be739f4
to
08c6779
Compare
@Rineee Added some more things. :) |
@@ -48,20 +20,18 @@ class ProposalSerializer(serializers.ModelSerializer): | |||
class Meta: | |||
model = Proposal | |||
fields = ( | |||
'budget', 'category', 'comment_count', 'created', | |||
'creator', 'is_archived', 'labels', 'moderator_feedback', | |||
'additional_item_badges_for_list_count', 'comment_count', |
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.
Cool, I never realized that you can serialize properties just like that!
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.
Totally love it! 🎉
Maybe someone else wants to check? But good to merge from my side once tests are fixed.
08c6779
to
d8ee032
Compare
test('displaying category badge', () => { | ||
render( | ||
<ListItemBadges | ||
badges={categoryBadge} | ||
/> | ||
) | ||
expect(screen.getByText('category1')).toBeTruthy() | ||
}) |
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 was a bit sad to delete tests, but as the component got a lot dumber with the serializer having all the info, they didn't make sense anymore. When I got it right, it only tests if the badges are there, right? We could still test if the moderator_statement is yellow and if the point_label has the icon or do the tests magically do that? @khamui Maybe you could have a look? (I will merge if the tests are actually all fine this time, though. 😅 )
…move badge properties to mixin to use in topics
I used the badge properties also in the detail views. But we need to check what to do with the budget badges on the tile.
So, questions for you @CarolingerSeilchenspringer :
We did use different solutions for all the different badges and all different idea derivates. With redoing the badges on the list, I put them into the same order and added the same behaviour everywhere:
That is fine, right?
But for the detail view, we only added the "budget not specified" to budgeting proposals, not kiezkasse. I also changed that and kiezkasse proposals are the same now. Also fine (just improving), right?
But finally the question we need a decision on: so far the budget is not displayed in the list when it is 0. https://meinberlin-dev.liqd.net/projekte/module/kiezkasse-4/?mode=list It would be easier if we could change that and also display the 0-label on the list:
But we can also remove it from the list. And keep it like it is now. Please tell us @CarolingerSeilchenspringer