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

fix(collective-page): add hover tooltip to collective names displayed in hero header #6678

Merged

Conversation

simonv3
Copy link
Contributor

@simonv3 simonv3 commented Jul 26, 2021

adds a component that shows a tooltip and truncates the name of a collective if its more than x
amount of characters, used to prevent collective names from showing as too long in the header of the
hero page

Happy to add tests to this, just wanted to get a directional "okay" first.

Resolve opencollective/opencollective#4495

Screenshots

Screenshot from 2021-07-26 11-12-35

image

@vercel
Copy link

vercel bot commented Jul 26, 2021

@simonv3 is attempting to deploy a commit to the Open Collective Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Jul 26, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

opencollective-styleguide – ./

🔍 Inspect: https://vercel.com/opencollective/opencollective-styleguide/9BsCE5KNmpik3gk9aP8wnaYJtHU7
✅ Preview: https://opencollective-styleguide-git-fork-simonv-699324-opencollective.vercel.app

opencollective-frontend – ./

🔍 Inspect: https://vercel.com/opencollective/opencollective-frontend/P8CfdZgriADvYki6tWnQ6tMyaxXB
✅ Preview: https://opencollective-frontend-git-fork-simonv3-b54e4a-opencollective.vercel.app

Copy link
Member

@SudharakaP SudharakaP left a comment

Choose a reason for hiding this comment

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

@simonv3 : Thanks much for the PR. I've added some comments.

@Betree : Could you take a look as well on this please. Thanks. 🙂

components/collective-page/hero/Hero.js Outdated Show resolved Hide resolved
components/collective-page/hero/OverflowCollectiveName.js Outdated Show resolved Hide resolved
components/collective-page/hero/Hero.js Outdated Show resolved Hide resolved
@SudharakaP SudharakaP requested a review from Betree July 27, 2021 17:17
@simonv3
Copy link
Contributor Author

simonv3 commented Jul 30, 2021

Wanted to set-up a unit test for the Hero component that would have caught the above errors, but it looks like there's some issues with dynamicImports and Jest (eg vercel/next.js#24566). Fixing that feels like it's beyond the scope of this?

@simonv3 simonv3 requested a review from SudharakaP August 4, 2021 16:27
Copy link
Member

@SudharakaP SudharakaP left a comment

Choose a reason for hiding this comment

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

@simonv3 : Thanks much for the changes and sorry for taking time on this. Some of our core members are on vacation so we are a bit short staffed.

Now I tested this PR and it seems that the tool-tip is appearing twice. Think there is two tool tips, one which is the default HTML tool-tip and the one you added. We need only one tool-tip; the user should ideally see the one you added and not the default tool-tip. 😉

Screenshot from 2021-08-09 10-13-32

@SudharakaP
Copy link
Member

Wanted to set-up a unit test for the Hero component that would have caught the above errors, but it looks like there's some issues with dynamicImports and Jest (eg vercel/next.js#24566). Fixing that feels like it's beyond the scope of this?

Yes, I don't think we have to do that here, maybe we can wait a bit as there's a fix for this in next; vercel/next.js#24751 which will probably take effect in some future release. Or we can also add a small E2E test for this at 04-collective.test.js. Let us wait for some feedback from @Betree. 🤔

@Betree : WDYT?

@Betree
Copy link
Member

Betree commented Aug 16, 2021

Wanted to set-up a unit test for the Hero component that would have caught the above errors, but it looks like there's some issues with dynamicImports and Jest (eg vercel/next.js#24566). Fixing that feels like it's beyond the scope of this?

Yes, I don't think we have to do that here, maybe we can wait a bit as there's a fix for this in next; vercel/next.js#24751 which will probably take effect in some future release. Or we can also add a small E2E test for this at 04-collective.test.js. Let us wait for some feedback from @Betree. thinking

@Betree : WDYT?

Yes, having a test in the Hero section of

seems more appropriate.

@simonv3
Copy link
Contributor Author

simonv3 commented Sep 13, 2021

Hey all, I'm sorry about the slow response - work picked up again and I likely won't have the time to get back to this.

@SudharakaP
Copy link
Member

Hey all, I'm sorry about the slow response - work picked up again and I likely won't have the time to get back to this.

No worries; thanks for your work. I'll complete the remaining part of this. 😉

… in hero header

adds a component that shows a tooltip and truncates the name of a collective if its more than x
amount of characters, used to prevent collective names from showing as too long in the header of the
hero page

to do with opencollective#4495
@SudharakaP SudharakaP force-pushed the fix/4495-long-fiscal-host-names branch from cc368c5 to 38e8e7e Compare September 13, 2021 18:05
@SudharakaP
Copy link
Member

@Betree : I've corrected the double tooltip problem I mentioned before; #6678 (review). Therefore I think this is all good to review and merge if you are okay. let me know. 👍🏽

components/LinkCollective.js Outdated Show resolved Hide resolved
components/collective-page/hero/Hero.js Outdated Show resolved Hide resolved
components/collective-page/hero/Hero.js Outdated Show resolved Hide resolved
@SudharakaP
Copy link
Member

@Betree : Thanks for the reviews. I've corrected the things you mentioned. Let me know if you see any further issues. 👍🏽

Copy link
Member

@Betree Betree left a comment

Choose a reason for hiding this comment

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

The code looks good

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

Successfully merging this pull request may close these issues.

Long Fiscal Host and Connected Collective name
3 participants