Start porting docs content #2735
Conversation
The current documentation does not have the required frontmatter and it uses components which need to be refactored or ported across as well. Because of this rendering will be totally broken while we work on adding the missing metadata and components.
…wn marketing speak
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -0,0 +1,66 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
This component looks like it's basically <Card> but with added variants. I don't think that's the right component to use here; instead I think we should be using <InfoBox> which already has most if not all of these variants and I think is a more correct fit for a callout (assuming I understand how you're using this correctly)
There was a problem hiding this comment.
I hadn't seen InfoBox, it looks like it and CopyButton aren't in Storybook so I missed them.
The icon and header are required props for InfoBox but were not for Callout so the interfaces are different. However, rather than duplicating the majority of the component I'll create a thin wrapper around it that sets default values for icon and header based on the type
There was a problem hiding this comment.
I'd just make those changes to InfoBox directly, it seems reasonable for the component to have those defaults
There was a problem hiding this comment.
Oh and <Callout /> supports an optional href prop to convert the whole thing into a <Link />, but looking at the existing docs we don't seem to ever actually use that functionality so we can just leave it out. If we need to link to somewhere from a within a <Callout /> we can just add it to the children.
| @@ -0,0 +1,96 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
There is a <CopyButton> component in the component library which implements the copy to clipboard functionality, I think you can just use that and simplify this component to just implement the address truncation
There was a problem hiding this comment.
Wrapped the existing <CopyButton /> component as it needs to support copying and navigating when clicked.
There was a problem hiding this comment.
You can take a look at https://github.com/pyth-network/pyth-crosschain/blob/main/apps/entropy-explorer/src/components/Address/index.tsx for a component which implements exactly this functionality for the WIP entropy explorer app, you'll see it used in a few places in the preview. Maybe we should extract that to the component library if it will become a more commonly used component?
There was a problem hiding this comment.
Yeah I guess it depends on wether or not we want the copy and the navigate to be two seperate controls. They were not in the previous implementation on existing docs site. Clicking on the link would copy and navigate.
I prefer the way you're doing it in that preview, I was just replicating what was there. But I'll change it.
There was a problem hiding this comment.
ahh got it, I never even realized they were the same control. I agree that's an odd way to handle it, but it's totally your call
| <span | ||
| className={styles.truncateToMiddle} | ||
| data-text-start={text.slice(0, Math.floor(text.length / 2))} | ||
| data-text-end={text.slice(Math.floor(text.length / 2) * -1)} |
There was a problem hiding this comment.
Nice, this is clever. Note that I believe there will be accessibility issues with only showing content in ::before and ::after so ideally you'd give this {text} as the children and then set @include theme.sr-only so that the content doesn't show up visually (you might want to read some a11y docs to confirms as I can't recall how accessibility devices handle pseudo-elements).
It would also be nice to be able to specify the minimum number of characters at the start & end here if possible.
There was a problem hiding this comment.
I sorta got the minimum number of characters working, it's 100% accurate for strings of all zeros 😅
|
|
||
| const { pressProps } = usePress({ | ||
| onPress: () => { | ||
| void writeClipboardText(address); |
There was a problem hiding this comment.
one minor point but I personally avoid void as the intent is not obvious when reading the code. To me, it's better to write doSomething().catch((error: unkonwn) => {/* no-op */}), which makes it very obvious that you are intentionally disregarding error handling. void has the same effect but does it implicitly and does not look like an intentional decision to disregard errors.
…ced code block Known issue in current version of Prettier. See: prettier/prettier#15740
| @@ -0,0 +1,171 @@ | |||
| export const CASE_VARIANTS = [ | |||
There was a problem hiding this comment.
So the problem with modules like this is that they are extremely locale specific and they make it a nightmare to localize codebases if we ever decide to do so. I generally recommend explicitly mapping strings rather than relying on clever tricks like this because mapping strings explicitly is simpler, drastically easier to localize, and also generally less brittle.
| neutral: "⦿", | ||
| info: "💡", | ||
| warning: "⚠️", | ||
| error: "❗", | ||
| data: "💾", | ||
| success: "🎉", |
There was a problem hiding this comment.
Should we use icons from phosphor-icons to be consistent with the rest of our iconography?
I also think you should add the default icons to the underlying InfoBox component directly, it makes sense to me that it would have sane defaults.
There was a problem hiding this comment.
Can do, the current content uses Emojis, but that is probably a requirement of the old framework rather than a conscious decision. I'm happy to swap them out
| ); | ||
| }; | ||
|
|
||
| export default CopyAddress; |
There was a problem hiding this comment.
did we decide we don't want to extract & reuse the same component from entropy-explorer for some reason? I think it would be better to share the same component given this is effectively identical functionality
There was a problem hiding this comment.
Oh I just saw your comment about their being a Copy component in the library, so that's what I used. I'll look at the explorer components and work on getting it extracted.
There was a problem hiding this comment.
Okay, so the <Address /> component in entropy-explorer is tightly coupled with EntropyDeployments. So I don't think we can extract and use that component. However, we could create a <CopyWithLink /> component that is responsible for the rendering and have <Address /> and the devhub use that component.
There was a problem hiding this comment.
Yeah that's exactly what I had in mind, the only coupling in that component to EntropyDeployments is to figure out the href value, so just leave that part in entropy-explorer and extract the rest of the component & styling to the component library
| // might differ slightly from the specified 'ch' value. | ||
| const style = { | ||
| "--min-chars-start-ch": `${minCharsStart.toString()}ch`, | ||
| "--min-chars-end-ch": `${minCharsEnd.toString()}ch`, |
There was a problem hiding this comment.
Nice, this is very clever, I like this solution a lot
Summary
Port across the price feed MDX files and display components
Rationale
There's still more content to be added, but this PR should also fix the Eslint errors that are currently being triggered on main.