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

Tooltip and full asset name #7828

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

609harsh
Copy link
Contributor

Proposed Changes

@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

@609harsh 609harsh requested a review from a team as a code owner May 15, 2024 12:23
Copy link

vercel bot commented May 15, 2024

@609harsh is attempting to deploy a commit to the Open Healthcare Network Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

netlify bot commented May 15, 2024

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 6b47289
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/665068a1788cc400096d8f41
😎 Deploy Preview https://deploy-preview-7828--care-egov-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nihal467
Copy link
Member

nihal467 commented May 22, 2024

@609harsh

image

  • Don't show this tooltip for all asset names. Show it only for assets with lengthy names, similar to the screenshot attached in the issue. When the user hovers over those cards, display the tooltip.
  • fix the cypress failure

@609harsh
Copy link
Contributor Author

@609harsh

image

  • Don't show this tooltip for all asset names. Show it only for assets with lengthy names, similar to the screenshot attached in the issue. When the user hovers over those cards, display the tooltip.
  • fix the cypress failure

Hi @rithviknishad how can 1st one be done? please suggest

@rithviknishad
Copy link
Member

As a simple fix, we can show tooltip only if name length is beyond certain length. Although it may not guarantee in all screen sizes and zoom levels.

Not sure if there are better alternative ways to achieve that fix via CSS. @skks1212 Any ideas?

@skks1212
Copy link
Member

If we follow conventions, I believe we should use a simple title prop rather than a tooltip for all names regardless of length.
We can also keep a toggle on the name. If you click/hover on it, rather than a tooltip, the full name is toggled. Just suggesting.
Keeping the popup dependant on length doesn't seem like a good idea to me
cc. @rithviknishad

@609harsh
Copy link
Contributor Author

609harsh commented Jun 1, 2024

@nihal467 what should be done next? Since displaying dynamic tooltip can only be possible by length of text but it will not insure all screen sizes give similar results. i guess we can go ahead with displaying tooltip to all names or remove it for all as most of the time valid asset name will be smaller.

@khavinshankar
Copy link
Member

@609harsh

can you try out what @skks1212 suggested

We can also keep a toggle on the name. If you click/hover on it, rather than a tooltip, the full name is toggled. Just suggesting.

and also fix the cypress issue

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.

Lengthy asset name is not properly handled in asset list page
5 participants