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: [DHIS2-15783] Tooltip on long working list names #3474

Merged
merged 7 commits into from
Dec 15, 2023

Conversation

eirikhaugstulen
Copy link
Contributor

@eirikhaugstulen eirikhaugstulen commented Nov 28, 2023

Summary:

  • Added a tooltip for TemplateSelectorChip
  • Wrapped it in a button to get keyboard focusability (not supported by UI)

@eirikhaugstulen eirikhaugstulen marked this pull request as ready for review December 4, 2023 08:43
@eirikhaugstulen eirikhaugstulen requested a review from a team as a code owner December 4, 2023 08:43
Copy link
Contributor

@superskip superskip left a comment

Choose a reason for hiding this comment

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

Looks like a decent solution 👍
Perhaps we can make a slight improvement by not showing the tooltip if a name has not been truncated? (I tried to see if ConditionalTooltip could be applied here, but it dosen't quite seem to fit. So I'm thinking more along the lines of using a plain conditional expression for achieving this.)

Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

Hey @eirikhaugstulen,
Can you add the same logic on the /search page inside the TemplateSelector component?
Thanks!

@eirikhaugstulen
Copy link
Contributor Author

Thanks for the review, @simonadomnisoru & @superskip!

Regarding conditionally rendering tooltips; there seems to be a lot of work to make this generic while using ConditionalTooltip. (Chips we can wrap in a separate button, while if we use DHIS2/UI buttons, things work a bit differently.)
If you check my latest commit, you can see how it could be implemented with a separate ConditionalTooltipForChip-component. Could you take a look and see if this is something you want?

Copy link
Contributor

@superskip superskip left a comment

Choose a reason for hiding this comment

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

Hey @eirikhaugstulen!
I think extracting the style- and ref-stuff into a separate component made the code look better 😄
I'm wondering if we should put the two conditional tooltip components in a common Tooltip folder. I also suggest renaming the new tooltip component to TooltipForChip (no need to make clear in the title that such tooltips can be disabled).

@eirikhaugstulen
Copy link
Contributor Author

Thanks @superskip, I implemented your changes!
I still feel like this new Tooltip could be named something Conditional as it takes in an enabled prop (required) and it might not show above a chip. But this is not a hill I'll die on, and TooltipForChip makes sense for now!

@superskip
Copy link
Contributor

superskip commented Dec 6, 2023

Very good @eirikhaugstulen! I only have one final minor refactor suggestion, which is to add an index file to the Tooltips folder. Please say no if you think it's too pointless.

EDIT: Regarding the enabled prop, we could maybe make that optional and true by default, if that solves your main concern with the shorter name?

Copy link
Contributor

@superskip superskip left a comment

Choose a reason for hiding this comment

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

After our discussion I agree with you it's fine as it is now, never mind last comment 🙂

Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

Regarding conditionally rendering tooltips; there seems to be a lot of work to make this generic while using ConditionalTooltip. (Chips we can wrap in a separate button, while if we use DHIS2/UI buttons, things work a bit differently.) If you check my latest commit, you can see how it could be implemented with a separate ConditionalTooltipForChip-component. Could you take a look and see if this is something you want?

The latest commits look great! Good job @eirikhaugstulen 👏

Copy link

github-actions bot commented Dec 7, 2023

Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

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

Tested successfully on 2.41,2.40.3,2.39.5,2.38.6 versions

@eirikhaugstulen eirikhaugstulen merged commit 6263aa8 into master Dec 15, 2023
37 checks passed
@eirikhaugstulen eirikhaugstulen deleted the eh/feat/DHIS2-15783-tooltip-for-long-wl branch December 15, 2023 14:01
dhis2-bot added a commit that referenced this pull request Dec 15, 2023
# [100.48.0](v100.47.3...v100.48.0) (2023-12-15)

### Features

* [DHIS2-15783] Tooltip on long working list names ([#3474](#3474)) ([6263aa8](6263aa8))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.48.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

5 participants