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

Fixes #4489: Adding last resort truncation to User Model's getShortName() function #4533

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

Bajszi97
Copy link
Contributor

This pull request closes issue #4489

by providing a last resort option to the getShortName() function within the User model. This option is necessary because the function fails to return anything in some cases. If the name is too long and cannot be split by spaces, it will be trimmed to the specified number of characters, followed by two dots ('..').

@ssddanbrown
Copy link
Member

Thanks for this @Bajszi97!

Could you tweak this to use three dots instead of two? To form a full ellipsis.
Additionally, could you tweak the truncation to take into account the added ellipsis (so reduce truncate length by three)?

It would be good to add a test to cover the originally raised issue in #4489, just a check to view a page with a comment and ensure the expected truncated name is visible.
Could be added to the existing CommentTest class.
Would you want to give that a go? Otherwise I can add that myself before merge.

@otherjoel
Copy link

Even better, use the actual ellipses character:

@Bajszi97
Copy link
Contributor Author

Could you tweak this to use three dots instead of two? To form a full ellipsis. Additionally, could you tweak the truncation to take into account the added ellipsis (so reduce truncate length by three)

Of course, I will change that. I have discarded the idea of reducing the truncated length, as it will cause problems on numbers smaller than three, even though it's very unlikely to call the function with such a small length. I will use the actual ellipses character, if it's considered better practice.

It would be good to add a test to cover the originally raised issue in #4489, just a check to view a page with a comment and ensure the expected truncated name is visible. Could be added to the existing CommentTest class. Would you want to give that a go? Otherwise I can add that myself before merge.

Yes i would. I have never done testing before, but feels like a good way to start. If i can't make it work, i will inform you here.

@Bajszi97
Copy link
Contributor Author

I have applied the changes you mentioned and managed to come up with something for the test, but I have some concerns regarding it.
I feel like there is a better way to get a user with long enough name for the test case, but i could not figure it out.
The other concern I have is that if the length provided to the function in the comment.blade view changes, this test may fail. However, it doesn't necessarily mean that something is wrong.

ssddanbrown added a commit that referenced this pull request Sep 13, 2023
- Updating formatting.
- Tweaked truncation to roughly match elipsis char to width used.
- Updated testing to use existing helpers, and ran check as admin user
  to avoid name conflicts.
@ssddanbrown ssddanbrown merged commit 83028f3 into BookStackApp:development Sep 13, 2023
@ssddanbrown
Copy link
Member

Thanks @Bajszi97, now merged for next feature release.
I made some tweaks during review in a452092. Mainly formatting and tweaking the test to use some existing helpers, and so the test was performed as another user to avoid the test potentially passing the test from the name elsewhere in the page (like the header bar).

I also adjusted the truncation to two, just because with @otherjoel's suggestion, I roughly measured the visual width of that character to be just over baseline character width, so I rounded up to consider it as 2 characters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants