-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add production/consumption to topic list and topic overview #2789
base: master
Are you sure you want to change the base?
Add production/consumption to topic list and topic overview #2789
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.
Hello there acass91! 👋
Thank you and congrats 🎉 for opening your first PR on this project! ✨ 💖
We will try to review it soon!
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.
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.
That's largely subjective. We could be going back and forth for days without a specification for the UI. Is there an exact font and font size that you want? I used the defaults for that table. Is there an example that doesn't use |
It might be. What's not subjective is that topic row height shouldn't be random and dynamic. Let's wait for the design in this case. |
Agreed, the row height can be fixed. Hacktoberfest label would be awesome, thanks. |
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.
- Please create custom Cell component for such logic - Cell API
- This component should not contain
<tr><td>
- only value
More info about Column Defs https://tanstack.com/table/v8/docs/guide/column-defs#accessor-functions
…splay-topic-throughput-stats
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.
…display-topic-throughput-stats
Kudos, SonarCloud Quality Gate passed! |
@acass91 thanks. We'll get back to your PR once we do a release, currently we're in a code freeze stage. Thanks! |
kafka-ui-react-app/src/components/Topics/List/ThroughputCell.tsx
Outdated
Show resolved
Hide resolved
@David-DB88 fix the rest when you have time please |
# Conflicts: # kafka-ui-react-app/src/components/common/Icons/ArrowDownIcon.tsx
@Haarolean i'll try |
} | ||
if (production === undefined) { | ||
return ( | ||
<Wrapper> |
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.
we can refactor this part in a tiny component. and use it in this conditions.
one question why are we checking if (production === undefined)
instead of if(!production)
.
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.
@Mgrdich i just change conditions take a look please if its normal ?
Kudos, SonarCloud Quality Gate passed! |
#2709
How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)