Did pluralize(...) and trim(...) so might as well do pluralizeAndTrim(...)
Details
Ran tests
Diff Detail
- Repository
- rCOMM Comm
- Branch
- landjune23 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
It might be worth adding a test where a length of a single item is higher than the limit
Done, added the "should trim the pluralized string to fit within maxLength" test to handle that case
lib/utils/text-utils.js | ||
---|---|---|
43 ↗ | (On Diff #13743) | @atul This change significantly alters the behavior described in a diff that introduced this code https://phab.comm.dev/D698. We need to make sure that the new behavior is better before changing it. Do you think the new behavior is better? At least we should check how it looks like in our UI - in this case it probably can be tested by checking how inline sidebar looks like when only one user with a long name responded to it. |
lib/utils/text-utils.js | ||
---|---|---|
43 ↗ | (On Diff #13743) |
I think so? Previously, if the 3-noun and 2-noun representation didn't fit within maxLength, we would go with the 1-noun representation even if it exceeded maxLength. Given that the name of the function is pluralizeAndTrim and that maxLength is an argument, I thought that the original intention was to cap the number of characters to maxLength. I think this was suggested in D698 as well:
Yeah you're right, I should have looked at different usages in the UI instead of looking at this function in isolation. I'll see what InlineSidebar looks like when a user with a long username responds, and I'll add a response here with what I find.
Agreed, I should have done that as well. |