-
-
Notifications
You must be signed in to change notification settings - Fork 36
Added Feature: Information description for each tab #349 #350
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
base: dev
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Failed checks @krishchvn |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds descriptive info tooltips to each detail panel/tab and standardizes header spacing.
- Introduces a reusable
InfoTooltip
component with CSS for hoverable descriptions - Integrates
InfoTooltip
into all panel headers (packs, lists, labeled, history, blocking, etc.) - Adjusts padding and margins in several panel and list CSS files for consistent header layout
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/common-components/info-tool-tip.jsx | New InfoTooltip component for hoverable descriptions |
src/common-components/info-tool-tip.css | Styling for tooltip visibility, positioning, and transitions |
src/detail-panels/packs/packs.jsx | Adds tooltip to "Starter Packs Created" header |
src/detail-panels/packs/packed.jsx | Adds tooltip to "Member of Packs" header |
src/detail-panels/lists/lists.jsx | Imports and adds tooltip to Lists panel |
src/detail-panels/lists/lists.css | Adjusts list header padding |
src/detail-panels/labeled/index.jsx | Adds tooltip to LabeledPanel header |
src/detail-panels/labeled/labeled.css | Refines margin and padding for labeled panel |
src/detail-panels/history/history-panel.jsx | Adds tooltip to History tab |
src/detail-panels/blocking/index.jsx | Adds tooltip to BlockingPanel header |
src/detail-panels/blocking-lists/blocking-lists-index.jsx | Adds tooltip and refactors inline styles in BlockingLists |
src/detail-panels/blocked-by/index.jsx | Adds tooltip to BlockedByPanel header |
src/detail-panels/blocked-by-lists/blocked-by-lists-index.jsx | Adds tooltip to BlockedByLists header |
src/detail-panels/block-panel-generic/block-panel-generic.css | Tweaks padding for blocking panel header |
Comments suppressed due to low confidence (2)
src/common-components/info-tool-tip.jsx:1
- [nitpick] Filename 'info-tool-tip.jsx' uses hyphens inconsistent with the
InfoTooltip
component name. Consider renaming the file to 'info-tooltip.jsx' to match the component naming convention.
import InfoIcon from '@mui/icons-material/Info';
src/detail-panels/lists/lists.jsx:60
- [nitpick] Tooltip text should start with an uppercase letter and end with a period for consistency. Consider updating to 'This page shows the lists you're a member of.'
<InfoTooltip text="this page shows the lists you're a member of" />
const InfoTooltip = ({ text }) => { | ||
return ( | ||
<div | ||
className="info-icon-wrapper" | ||
tabIndex="0" | ||
aria-describedby="tooltip-text" | ||
> | ||
<InfoIcon className="info-icon" /> | ||
<span className="tooltip-text">{text}</span> |
Copilot
AI
Jul 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a static id 'tooltip-text' for aria-describedby can lead to duplicate IDs when multiple tooltips are rendered. Consider generating unique IDs per instance or using aria-label with a role="tooltip"
on the tooltip element to improve screen reader accessibility.
const InfoTooltip = ({ text }) => { | |
return ( | |
<div | |
className="info-icon-wrapper" | |
tabIndex="0" | |
aria-describedby="tooltip-text" | |
> | |
<InfoIcon className="info-icon" /> | |
<span className="tooltip-text">{text}</span> | |
import { useId } from 'react'; | |
const InfoTooltip = ({ text }) => { | |
const tooltipId = useId(); | |
return ( | |
<div | |
className="info-icon-wrapper" | |
tabIndex="0" | |
aria-describedby={tooltipId} | |
> | |
<InfoIcon className="info-icon" /> | |
<span id={tooltipId} className="tooltip-text">{text}</span> |
Copilot uses AI. Check for mistakes.
return ( | ||
<> | ||
<div | ||
style={{ fontWeight: '400', paddingLeft: '0.5em', paddingTop: '0.3em' }} |
Copilot
AI
Jul 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This inline style block is duplicated across multiple panels. Consider extracting it into a shared CSS class or styled component to reduce duplication and improve maintainability.
Copilot uses AI. Check for mistakes.
<div | ||
style={{ | ||
fontWeight: '400', | ||
// paddingBottom: '0.2em', |
Copilot
AI
Jul 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Commented-out style rules can clutter the code. If this padding is no longer needed, remove the commented line to keep the code clean.
// paddingBottom: '0.2em', |
Copilot uses AI. Check for mistakes.
@thieflord06 Should i make changes according to copilot's review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have specific changes here as much as just some general remarks.
One overall point I want to make: I know this was just literally implementing what was suggested in #349, but I think that was a bad suggestion from a design standpoint. The whole point of a tooltip is to make extra information available without taking up space when it isn't needed. This completely fails at that.

Look at all that extra unused space we added to the page in service of this single feature. Also, it's not clear that the info icon is associated with the selected tab, because it's so physically distant from the tab itself (with the exception of the very first tab, or some scrolling scenarios).
As is, this would be better off not as a tooltip, but just as an informational paragraph in the new space. Something that's always visible and requires zero interaction to discover. Something that at least makes use of the space being reserved by this change.
But that's not even the right way to handle this. The information should be something directly connected to the tab itself, and it should take up almost zero new space in the layout.
<div | ||
className="info-icon-wrapper" | ||
tabIndex="0" | ||
aria-describedby="tooltip-text" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here when there's no element with an id of tooltip-text
for it to refer to?
{(isLoadingTotal && !listsTotal) && <span style={{ opacity: 0.5 }}>{"Counting block lists..."}</span>} | ||
{listsTotal ? | ||
<h3 className="lists-header"> | ||
<div style={{ fontWeight: '400', paddingBottom: '0.2em' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with copilot: if this same div and style is repeated everywhere we use the tooltip component, then just include it inside the tooltip directly
paddingTop: '0.3em', | ||
}} | ||
> | ||
<InfoTooltip text="this page shows panel members you have blocked via lists" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the wording on some of these are confusing to me. what is a "panel member"?
these are also all worded using the 2nd person voice, assuming the reader is only viewing their own profile: "members YOU have blocked"
I feel it may also add some confusion if someone is relying on these to understand the tab contents from a different profile than their own.
@krishchvn any update on this? |
@krishchvn This still needs to be reworked. |
Created a info tab, on hover, shows a small description of tab