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

DS implementation - Table of Contents #13156

Merged
merged 13 commits into from
Aug 6, 2024
Merged

DS implementation - Table of Contents #13156

merged 13 commits into from
Aug 6, 2024

Conversation

pettinarip
Copy link
Member

@pettinarip pettinarip commented Jun 12, 2024

@github-actions github-actions bot added the tooling 🔧 Changes related to tooling of the project label Jun 12, 2024
Copy link

netlify bot commented Jun 12, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 8030741
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/66b293bfbd58af000833409d
😎 Deploy Preview https://deploy-preview-13156--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 43 (🔴 down 9 from production)
Accessibility: 93 (no change from production)
Best Practices: 88 (🔴 down 10 from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@pettinarip pettinarip changed the title [WIP] DS implementation - Table of Contents DS implementation - Table of Contents Jun 13, 2024
@wackerow
Copy link
Member

This can likely replace #11957, as it fixes the indicator alignment issue... This PR also incorporates the new styling, so it should be preferred.

That PR got blocked when trying to polish for both LTR and RTL... this seems to also currently be a bug here: https://deploy-preview-13156--ethereumorg.netlify.app/fa/developers/docs/transactions/

image

@pettinarip
Copy link
Member Author

This can likely replace #11957, as it fixes the indicator alignment issue... This PR also incorporates the new styling, so it should be preferred.

That PR got blocked when trying to polish for both LTR and RTL... this seems to also currently be a bug here: https://deploy-preview-13156--ethereumorg.netlify.app/fa/developers/docs/transactions/

Hmm is weird because on Storybook is working fine. For some reason, insetInlineStart is being converted to right or left by Chakra or emotion, but on the site, it is not doing that...is always left 🤷🏼

@TylerAPfledderer do you have any ideas on what could be going on here?

@wackerow
Copy link
Member

@pettinarip This is looking good now! Still in draft mode though... is this ready for a formal review yet? Flip to "Ready for review" and @ me when ready and will try to get this in.

@wackerow
Copy link
Member

Only thing I'm noticing now (not a blocker imo) is the direction of the chevron's for the accordion, RTL vs LTR:

image image

Notice they point RIGHT no matter what... in LTR (ie English) these should point Right, but in RTL (ie. Farsi, Arabic, Hebrew, Urdu) these should point Left.

We can address this separately though to prevent scope creep... I suspect there may be a couple annoyances fixing that when it comes to the animation.

@wackerow
Copy link
Member

Oops, never mind! Wrong PR... my previous comments are fixed in #13063, please disregard

@wackerow wackerow added the needs design approval 🧑‍🎨 Approval from a designer is needed before merging label Jun 21, 2024
@wackerow wackerow requested a review from nloureiro June 21, 2024 14:46
@pettinarip pettinarip marked this pull request as ready for review June 21, 2024 14:46
Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

@pettinarip Looks great, just spotting one color mis-match.

{t("edit-page")}
</ButtonLink>
)}
<Box textTransform="uppercase">{t("on-this-page")}</Box>
Copy link
Member

Choose a reason for hiding this comment

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

Figma:
image

Preview:
image
image

Suggested change
<Box textTransform="uppercase">{t("on-this-page")}</Box>
<Box textTransform="uppercase" color="body.medium">
{t("on-this-page")}
</Box>

type Story = StoryObj<typeof meta>

export const Default: Story = {
render: () => <TableOfContents slug="#web3" items={tocItems} maxDepth={2} />,
Copy link
Member

Choose a reason for hiding this comment

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

9:11:34 AM: Failed to compile.
9:11:34 AM: 
9:11:34 AM: ./src/components/TableOfContents/TableOfContents.stories.tsx:94:34
9:11:34 AM: Type error: Type '{ slug: string; items: ToCItem[]; maxDepth: number; }' is not assignable to type 'IntrinsicAttributes & BoxProps & { items: ToCItem[]; maxDepth?: number | undefined; editPath?: string | undefined; hideEditButton?: boolean | undefined; isMobile?: boolean | undefined; }'.
9:11:34 AM:   Property 'slug' does not exist on type 'IntrinsicAttributes & BoxProps & { items: ToCItem[]; maxDepth?: number | undefined; editPath?: string | undefined; hideEditButton?: boolean | undefined; isMobile?: boolean | undefined; }'.
9:11:34 AM:   92 |
9:11:34 AM:   93 | export const Default: Story = {
9:11:34 AM: > 94 |   render: () => <TableOfContents slug="#web3" items={tocItems} maxDepth={2} />,
9:11:34 AM:      |                                  ^
9:11:34 AM:   95 | }
9:11:34 AM:   96 |
9:11:34 AM: error Command failed with exit code 1. (https://ntl.fyi/exit-code-1)

cc: @pettinarip Getting an error here... slug is not a prop on TableOfContents, do we need this here?

// src/components/TableOfContents/index.tsx
export type TableOfContentsProps = BoxProps & {
  items: Array<ToCItem>
  maxDepth?: number
  editPath?: string
  hideEditButton?: boolean
  isMobile?: boolean
}

@wackerow wackerow merged commit c33bdc4 into dev Aug 6, 2024
8 of 10 checks passed
@wackerow wackerow deleted the ds-toc branch August 6, 2024 22:57
This was referenced Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs design approval 🧑‍🎨 Approval from a designer is needed before merging tooling 🔧 Changes related to tooling of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TableOfContents
3 participants