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

[#6519] label on tiles #4542

Merged
merged 1 commit into from
Oct 14, 2022
Merged

[#6519] label on tiles #4542

merged 1 commit into from
Oct 14, 2022

Conversation

khamui
Copy link
Contributor

@khamui khamui commented Oct 4, 2022

No description provided.

@khamui khamui force-pushed the kt-2022-10-label-on-tiles branch from 18c4a08 to ce6c9bd Compare October 5, 2022 12:55
@khamui khamui changed the title WIP [#6519] label on tiles [#6519] label on tiles Oct 5, 2022
@khamui khamui changed the title [#6519] label on tiles WIP [#6519] label on tiles Oct 5, 2022
@khamui
Copy link
Contributor Author

khamui commented Oct 5, 2022

oh, wip again, fixing tests

@khamui khamui force-pushed the kt-2022-10-label-on-tiles branch from ce6c9bd to 12f6a40 Compare October 5, 2022 15:11
@khamui khamui changed the title WIP [#6519] label on tiles [#6519] label on tiles Oct 5, 2022
Copy link
Contributor

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

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

Yes, looks good and works!
But I am unsure about the isTruthy and if we shouldn't find another solution.
And I think, in the story, we were meant to cut very long categories and labels after a certain amount of chars and append ...

Comment on lines 25 to 31
// Before passing this data to 'ListItemBadges' component
// it needs to be normalized as the data types are different
// category: null || object
// labels: [] || array
// budget: 0 || number
// pointLabel: '' || string
// modFeedback: [null, null] || array
Copy link
Contributor

Choose a reason for hiding this comment

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

Uh. This makes sense, but shouldn't we change the serializer to give better values? We will have to change this for TS anyway. And in a lot of places. Maybe here is a good place to start and find out how we would like to get data from the API that can be there or not? We have all the examples you can think of in one place. :p

Copy link
Contributor Author

@khamui khamui Oct 13, 2022

Choose a reason for hiding this comment

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

@fuzzylogic2000 that is true. I will look into how to send them same from the serializer

@fuzzylogic2000
Copy link
Contributor

Oh, and I thought of another thing: is that the right order of the labels? Shouldn't the labels (Merkmale) be last as only these can have multiple labels and all the moderator's status thing should probably always be shown?

…omponent to support eliding (x More)

rework after review: normalize None-case for fields that are being used as badges. And some smaller fixes
@khamui khamui force-pushed the kt-2022-10-label-on-tiles branch from 12f6a40 to b1c4a96 Compare October 13, 2022 15:13
Copy link
Contributor

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

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

Nice!

@fuzzylogic2000 fuzzylogic2000 merged commit bba9aca into main Oct 14, 2022
@fuzzylogic2000 fuzzylogic2000 deleted the kt-2022-10-label-on-tiles branch October 14, 2022 07:46
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