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

feat(NcCounterBubble): add count prop for humanized count display #5863

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jul 24, 2024

☑️ Resolves

Currently, the maximal number is limited via ... ellipsis. It doesn't make sense for numbers. And it is broken on Nextcloud 30 styles.

image

In general, having a slot doesn't make much sense for me. As the component is supposed to show a count, it is better to work with it as a number, not any content.

Instead, add an explicit count humanize output using:

  • Added count prop for the value
  • Humanize output with localization by default
  • Even humanized output from the default slot content, if a plain number is passed
  • Mark default slot as deprecated in flavor of count or NcChip
  • Added raw prop to disable this behavior
  • Improved docs
  • Added tests

🖼️ Screenshots

With new count prop

English Chinese
image image

With old slot

image

Docs for styling

image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@ShGKme ShGKme added enhancement New feature or request 3. to review Waiting for reviews labels Jul 24, 2024
@ShGKme ShGKme added this to the 8.16.0 milestone Jul 24, 2024
@ShGKme ShGKme self-assigned this Jul 24, 2024
Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Very nice! :)

Is it possible to have this be default and not a choice/prop? That is, if a counter bubble is used anywhere, it should never go up further than 1000.

What about using 1K+ instead of 999+?

I'd prefer 999+ as that's more universally localized and a bit clearer that it's "a ton".

@ShGKme
Copy link
Contributor Author

ShGKme commented Jul 24, 2024

Is it possible to have this be default and not a choice/prop? That is, if a counter bubble is used anywhere, it should never go up further than 1000.

The limit is 999 by default. However, it will work only if you pass the number as a count value, not as a custom HTML content.

Changing the behavior of the current custom content rendering would be more breaking and also dirty implementation-wise.

@susnux
Copy link
Contributor

susnux commented Jul 24, 2024

Is it possible to have this be default and not a choice/prop?

It is 999 by default, which in most cases makes sense, but sometime even 99 would be ok, so we should not hard code this but just have a meaningful default, no?

@ShGKme ShGKme force-pushed the feat/nc-counter-bubble--limit branch 3 times, most recently from 157de2a to 7ce91df Compare July 24, 2024 14:14
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Rest is good!

@susnux susnux requested a review from jancborchardt August 1, 2024 12:02
@susnux susnux force-pushed the feat/nc-counter-bubble--limit branch from 7ce91df to bb5e2be Compare August 5, 2024 12:39
@Antreesy Antreesy force-pushed the feat/nc-counter-bubble--limit branch from bb5e2be to ba97433 Compare August 5, 2024 13:28
@Antreesy Antreesy modified the milestones: 8.16.0, 8.17.0 Aug 5, 2024
@marcelklehr
Copy link
Contributor

I would like to stress that numbers well above 1000 are a regular occurence. I would like to keep support for strings to allow consumers to pass things like "12.5k" if they so choose

@st3iny
Copy link
Contributor

st3iny commented Aug 6, 2024

As far as I can see, you can still use the slot for custom strings. It was only deprecated for now. Removing it is a breaking change and implies a major bump.

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 6, 2024

@marcelklehr do you have an example, where NcCounterBubble is used in such a way?

@marcelklehr
Copy link
Contributor

I use it in the bookmarks app to show bookmarks counts for folders and menu items

@jancborchardt
Copy link
Contributor

Ok, then let’s do it as you suggested (and also as e.g. Twitter does for favs and retweets):

  • "1K" from 1000 on
  • "1M" from 1 million on
  • Maximum 1 decimal point

Then it fulfills multiple requirements:

  • Still keeps an actual count (what @marcelklehr was saying)
  • Shows there are a ton of things (what the "999+" was communicating)
  • Doesn’t change small digits in large numbers

Sounds good @ShGKme @marcelklehr @susnux?

@susnux
Copy link
Contributor

susnux commented Aug 19, 2024

Sounds good for me :)

(While I personally would prefer a small k but a capital M to align with unit prefixes)

@jancborchardt
Copy link
Contributor

@susnux small "k" and capital "M" sounds good to me too, it seems like the standard. Not sure what the specific reason to Twitter using a capital K is.

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 19, 2024

Before I polish it, @susnux What do you think about this solution?

@ShGKme ShGKme force-pushed the feat/nc-counter-bubble--limit branch 3 times, most recently from 4906065 to 2f5a0a9 Compare August 19, 2024 20:27
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 19, 2024

  • Rebased onto master
  • Changed behavior:
    • Instead of + and limit -- humanize using Intl compact format
    • Also humanize plain number from slot content

@ShGKme ShGKme marked this pull request as ready for review August 19, 2024 20:28
@ShGKme ShGKme requested a review from susnux August 19, 2024 20:28
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 19, 2024

@jancborchardt Updated, see screenshots

@ShGKme ShGKme force-pushed the feat/nc-counter-bubble--limit branch from 2f5a0a9 to fe8eb1f Compare August 19, 2024 20:31

Note, if you pass a plain number to the default slot, without raw prop it will be humanized like via `count` prop. This feature is **deprecated** and will be removed in the future. Prefer using `count` prop for numbers with humanization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we deprecate the whole default slot?
As the counter function is done by the prop, using the slot without the counter handling feels like you want to use NcChip or NcUserBubble.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done
image

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW just for my reasoning:
From my point of view it makes no sense that we have so many components supporting the same use case.
This one is specialized for counting / showing a count, so we should stick with that behavior.

If there are complains about this, we still could add a unit prop where you can put n('row', 'rows') and the result would be 1 row, 99 rows, 1k rows. But lets see if this is needed.

@ShGKme ShGKme added the deprecation Related to the deprecation of anything available on the public API label Aug 20, 2024
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 20, 2024

/backport to next

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Awesome @ShGKme, sorry for the tears :D

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Looks good code-wise now!

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Super cool!

Maybe it makes sense to provide humanizeCount as part of @nextcloud/l10n?

@susnux
Copy link
Contributor

susnux commented Aug 20, 2024

(just needs to be squashed (fixup commit))

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 20, 2024

Maybe it makes sense to provide humanizeCount as part of @nextcloud/l10n?

I'm not sure. It's basically a browser feature, just a default compact format + getting locale from l10n.

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the feat/nc-counter-bubble--limit branch from 00f3adb to a756430 Compare August 20, 2024 10:36
@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 20, 2024

Rebased onto master and squashed

@ShGKme
Copy link
Contributor Author

ShGKme commented Aug 20, 2024

Maybe it makes sense to provide humanizeCount as part of @nextcloud/l10n?

Maybe makes sense to make it easier for developers

@ShGKme ShGKme enabled auto-merge August 20, 2024 10:37
@ShGKme ShGKme merged commit 756d004 into master Aug 20, 2024
19 checks passed
@ShGKme ShGKme deleted the feat/nc-counter-bubble--limit branch August 20, 2024 10:38
@susnux susnux mentioned this pull request Aug 20, 2024
@ShGKme ShGKme changed the title feat(NcCounterBubble): add count and limit props instead of slot feat(NcCounterBubble): add count prop for humanized count display Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews deprecation Related to the deprecation of anything available on the public API enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NcCounterBubble just keeps counting up
6 participants