Skip to content

chore(price-feed): added update parameters as a column in each table for EVM #739

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 195 additions & 0 deletions components/SponsoredFeedsTable.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
import { useState } from "react";
import CopyIcon from "./icons/CopyIcon";

interface SponsoredFeed {
name: string;
priceFeedId: string;
updateParameters: string;
}

interface SponsoredFeedsTableProps {
feeds: SponsoredFeed[];
networkName: string;
}

export const SponsoredFeedsTable = ({
feeds,
networkName,
}: SponsoredFeedsTableProps) => {
const [copiedId, setCopiedId] = useState<string | null>(null);

const copyToClipboard = (text: string) => {
navigator.clipboard.writeText(text).then(() => {
setCopiedId(text);
setTimeout(() => setCopiedId(null), 2000);
});
};

// Calculate parameter statistics
const paramCounts = feeds.reduce((acc, feed) => {
acc[feed.updateParameters] = (acc[feed.updateParameters] || 0) + 1;
return acc;
}, {} as Record<string, number>);
Comment on lines +29 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor nit but I tend to find this kind of thing is more easy to understand when implemented as:

const paramCounts = Object.fromEntries(
  Object
    .entries(Object.groupBy(feeds, feed => feed.updateParameters))
    .map(([updateParameters, feeds]) => [updateParameters, feeds.length])
)

I will also typically create a mapValues helper (which really should be in the ES standard IMO and probably will be eventually) which I use all the time and makes the code more concise:

const paramCounts = mapValues(
  Object.groupBy(feeds, feed => feed.updateParameters)),
  feeds => feeds.length
)

For some reason, reduce seems to be a hard operation for a lot of folks to easily grok so I've started to move away from using it except in very specific scenarios. Here is a decent writeup for an eslint rule that I use nowadays that has some links to some threads on the topic.


const defaultParams = Object.entries(paramCounts).sort(
([, a], [, b]) => b - a
)[0][0];

// Calculate table height based on number of items
// Each row is approximately 40px (py-2 = 8px top + 8px bottom + content height)
// Header is approximately 48px (py-2 = 8px top + 8px bottom + font height)
// Show 7 rows by default, then scroll - but maintain consistent minimum height
const maxVisibleRows = 7;
const shouldScroll = feeds.length > maxVisibleRows;
const rowHeight = 56; // Increased row height to account for actual content height
const headerHeight = 48; // Header height in pixels
const exactTableHeight = `${headerHeight + maxVisibleRows * rowHeight}px`; // Exact height for 7 rows
const tableHeight = shouldScroll ? exactTableHeight : "auto"; // Use exact height for scrollable tables
Comment on lines +38 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

I really dislike this kind of thing, it's very brittle (i.e. it's a pain to ensure every change is tested across device sizes and it's easy to do something that changes the calculations and not know it breaks the UI) and it's very easy to run into render issues on different platforms where size calculations have slight differences.

It also feels unnecessarily complex for what you're looking to accomplish.

In this case, what I'd suggest doing is just adding the max-height css property to the table


return (
<div className="my-6">
<p className="mb-3">
The price feeds listed in the table below are currently sponsored in{" "}
<strong>{networkName}</strong>.
</p>

<div className="border border-gray-200 dark:border-gray-700 rounded-lg">
{/* Summary bar */}
<div className="bg-blue-50 dark:bg-blue-900/20 px-3 py-2 border-b border-gray-200 dark:border-gray-600">
<div className="flex flex-wrap items-center gap-4 text-sm">
<div className="flex items-center gap-1.5">
<div className="w-1.5 h-1.5 bg-green-500 rounded-full flex-shrink-0"></div>
<span className="font-medium">Default:</span>
<span
dangerouslySetInnerHTML={{
__html: defaultParams.replace("<br/>", " / "),
}}
/>
<span className="text-gray-500">
({paramCounts[defaultParams]})
</span>
</div>
{Object.entries(paramCounts)
.filter(([params]) => params !== defaultParams)
.map(([params, count]) => (
<div key={params} className="flex items-center gap-1.5">
<div className="w-1.5 h-1.5 bg-orange-500 rounded-full flex-shrink-0"></div>
<span className="font-medium">Exception:</span>
<span
dangerouslySetInnerHTML={{
__html: params.replace("<br/>", " / "),
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit strange to me, is there any reason why params here needs to be free text with html embedded? This is not coming from a CMS or any external source, so there shouldn't be any reason to pass around html as strings. I'd suggest changing the format of updateParameters -- either pass around jsx fragments, or better yet, since it looks like the only difference in the entries is the heartbeat length & the deviation, just make updateParameters something like { heartbeatLength: number, deviation: number }

}}
/>
<span className="text-gray-500">({count})</span>
</div>
))}
</div>
</div>

{/* Table */}
<div className="overflow-x-auto">
<div
className={`${shouldScroll ? "overflow-y-auto" : ""}`}
style={{ height: tableHeight }}
>
<table className="w-full text-sm min-w-full">
<thead className="sticky top-0 bg-gray-50 dark:bg-gray-800 z-30">
<tr>
<th className="text-left px-3 py-2 font-semibold text-gray-900 dark:text-gray-100 border-b border-gray-200 dark:border-gray-600 min-w-[100px]">
Name
</th>
<th className="text-left px-3 py-2 font-semibold text-gray-900 dark:text-gray-100 border-b border-gray-200 dark:border-gray-600 min-w-[400px]">
Price Feed Id
</th>
<th className="text-left px-3 py-2 font-semibold text-gray-900 dark:text-gray-100 border-b border-gray-200 dark:border-gray-600 min-w-[200px]">
Update Parameters
</th>
</tr>
</thead>
<tbody className="bg-white dark:bg-gray-900">
{feeds.map((feed, index) => {
const isDefault = feed.updateParameters === defaultParams;
const prevFeed = feeds[index - 1];
const isFirstInGroup =
!prevFeed ||
prevFeed.updateParameters !== feed.updateParameters;

return (
<tr
key={feed.priceFeedId}
className={`border-b border-gray-100 dark:border-gray-700 hover:bg-gray-50 dark:hover:bg-gray-800/30 ${
isFirstInGroup
? "sticky top-12 bg-white dark:bg-gray-900 z-20 shadow-sm"
: ""
}`}
>
<td className="px-3 py-2 align-top">
<span className="font-medium text-gray-900 dark:text-gray-100">
{feed.name}
</span>
</td>
<td className="px-3 py-2 align-top">
<div className="flex items-start gap-2">
<code className="text-xs font-mono text-gray-600 dark:text-gray-400 flex-1 break-all leading-relaxed">
{feed.priceFeedId}
</code>
<button
onClick={() => copyToClipboard(feed.priceFeedId)}
className="p-1 hover:bg-gray-200 dark:hover:bg-gray-600 rounded flex-shrink-0 mt-0.5"
title="Copy Price Feed ID"
>
{copiedId === feed.priceFeedId ? (
<span className="text-green-500 text-xs font-bold">
</span>
) : (
<CopyIcon className="w-3 h-3 text-gray-400" />
)}
</button>
</div>
</td>
<td className="px-3 py-2 align-top">
{isFirstInGroup ? (
<div className="flex items-start gap-1.5">
<div
className={`w-1.5 h-1.5 rounded-full mt-1 flex-shrink-0 ${
isDefault ? "bg-green-500" : "bg-orange-500"
}`}
></div>
<span
className={`text-xs leading-relaxed font-medium ${
isDefault
? "text-gray-700 dark:text-gray-300"
: "text-orange-600 dark:text-orange-400"
}`}
dangerouslySetInnerHTML={{
__html: feed.updateParameters,
}}
/>
</div>
) : (
<div className="flex items-start gap-1.5 text-gray-400 text-xs">
<div className="w-1.5 h-1.5 bg-green-500 rounded-full mt-1 flex-shrink-0"></div>
<span>Same as above</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'm not a fan of this. I know the repetition is obnoxious, but when I'm looking up data, I'd much rather see repetition, but to have the value I'm looking for right where I'm looking, than to see "same as above" and then have to go cross reference another row.

My suggestion would be to just use the isFirstInGroup style for every entry.

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case, then shouldn't we keep these update parameters in the top line?

Copy link
Member

Choose a reason for hiding this comment

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

But if we do that, then the experience wouldn't be consistent between all the table.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow but from the screenshot I don't see any reason to get rid of the top line or the green/red dots, it's a helpful way to highlight the data that differs from baseline.

</div>
)}
</td>
</tr>
);
})}
</tbody>
</table>
</div>
</div>

{/* Show count indicator when scrolling is needed */}
{shouldScroll && (
<div className="px-3 py-1 bg-gray-50 dark:bg-gray-800 border-t border-gray-200 dark:border-gray-600 text-xs text-gray-500 text-center">
Showing {maxVisibleRows} of {feeds.length} feeds • Scroll to see
more
</div>
)}
</div>
</div>
);
};
Loading
Loading