-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat(cxl-ui): add cxl-stats component #291
Conversation
3ef1ce0
to
4ef2acf
Compare
@heshfekry The stats count is meant to change the count of items there is in the row. For now there is data for 4 items (max amount in the design) and the count is there to just show what would happen if there is less than 4 items. |
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.
Component has max value of 4 items in row which is sufficient for current design. I'm just wondering if it's worth making this component more universal and increase the limit.
@heshfekry what do you think?
@kertuilves what's your estimation on providing such functionality:
- for max 8 items,
- for any amount of items?
<p class="stats-title">${unsafeHTML(el.title)}</p> | ||
<h3 class="stats-count">${el.count}</h3> |
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 semantically incorrect. Title should use header element and count <p>
. It might seem natural to use <h*>
because it's bigger by styles, but styles can and should be adjusted.
Also please use <h4>
instead of <h3>
.
Dont see the need just yet. If its less than 2hrs of work sure. Otherwise can just duplicate conponent of max 4 to create different stat bundles of max 4 per block. |
4ef2acf
to
51fe20d
Compare
I made the changes so when the count is updated and it's bigger than 4, then it just adds last item again and again. |
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.
Looking good 👍 It's almost ready to merge, just minor tweaks needed.
Commit message:
- feat: add dashboard stats component
+ feat(cxl-ui): add `cxl-stats` component
51fe20d
to
723fe39
Compare
cxl-stats
component
cxl-stats
component
https://app.clickup.com/t/861n0qjxj