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

Limit maximum tooltip width to prevent overflow #29210

Merged

Conversation

normalid-awa
Copy link
Contributor

Replace the default IHasTooltip with IHasCustomTooltip<LocalisableString> and use the new custom tooltip CommentTooltip with TextFlowContainer to limit the width of the tooltip.

Master image image
PR image image

@bdach
Copy link
Collaborator

bdach commented Aug 1, 2024

I dunno. This is not worth a custom tooltip to me

If we're going to be chasing these instance by instance then I'd rather just honestly see OsuTooltip have a fixed max width and use a TextFlowContainer everywhere to avoid this. Or Truncate set, either or.

Curious of @ppy/team-client opinion.

@smoogipoo
Copy link
Contributor

Agree with the above, should be fairly easy to make it use TFC.

@normalid-awa normalid-awa marked this pull request as draft August 2, 2024 02:52
@pull-request-size pull-request-size bot added size/XS and removed size/M labels Aug 2, 2024
@pull-request-size pull-request-size bot added size/S and removed size/XS labels Aug 2, 2024
@normalid-awa normalid-awa changed the title Fix the visual bug of CommentAuthorLine tooltip text width not being truncate. Limit OsuTooltip maximum width to prevent overflow Aug 2, 2024
@normalid-awa normalid-awa changed the title Limit OsuTooltip maximum width to prevent overflow Limiting the OsuTooltip maximum width to prevent overflow Aug 2, 2024
@normalid-awa normalid-awa marked this pull request as ready for review August 2, 2024 04:17
@normalid-awa
Copy link
Contributor Author

I've changed the OsuTooltip text to TFC and limited the width, so it should be fixing all potential visual bugs (tooltip overflow).

osu.Game/Graphics/Cursor/OsuTooltipContainer.cs Outdated Show resolved Hide resolved
osu.Game/Graphics/Cursor/OsuTooltipContainer.cs Outdated Show resolved Hide resolved
smoogipoo
smoogipoo previously approved these changes Aug 3, 2024
@smoogipoo smoogipoo requested a review from peppy August 3, 2024 10:40
@smoogipoo
Copy link
Contributor

Requesting one more review to confirm I haven't gone off the rails with my suggestions (especially maximum size of 500).

@peppy
Copy link
Sponsor Member

peppy commented Aug 3, 2024

@smoogipoo how are you testing this? is there a test scene i'm missing?

@normalid-awa
Copy link
Contributor Author

normalid-awa commented Aug 4, 2024

@smoogipoo how are you testing this? is there a test scene i'm missing?

I think there's no test scene to test the tooltips, I think I could probably open a pr to implement that.

@normalid-awa
Copy link
Contributor Author

@smoogipoo how are you testing this? is there a test scene i'm missing?

I've written the unit test now, not sure if I have to open a new pr or just push to this branch

@peppy
Copy link
Sponsor Member

peppy commented Aug 4, 2024

@normalid-awa you can commit here

@normalid-awa
Copy link
Contributor Author

normalid-awa commented Aug 4, 2024

@normalid-awa you can commit here

Unit test was pushed, One of the test was temporarily disabled due to the o!f issue. ref discussion

@peppy peppy enabled auto-merge August 5, 2024 08:01
@peppy
Copy link
Sponsor Member

peppy commented Aug 5, 2024

@normalid-awa please check the changes i made to your tests to improve things slightly. otherwise looks fine!

@normalid-awa
Copy link
Contributor Author

@normalid-awa please check the changes i made to your tests to improve things slightly. otherwise looks fine!

Oh...., I was a bit confused at that point, the test should be enable but only disabled the problem case. The changes looks good to me!

@peppy peppy disabled auto-merge August 5, 2024 09:09
@peppy peppy merged commit f41bab0 into ppy:master Aug 5, 2024
11 of 13 checks passed
@bdach bdach changed the title Limiting the OsuTooltip maximum width to prevent overflow Limit maximum tooltip width to prevent overflow Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants