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

feat: implement logic to parse the toc.toml and render it #1329

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

branberry
Copy link
Collaborator

Stories/Links:

DOP-NNNN

Current Behavior:

Put a link to current staging or production behavior, if applicable

Staging Links:

Put a link to your staging environment(s), if applicable

Notes:

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for docs-frontend-dotcomstg ready!

Name Link
🔨 Latest commit dd0ad5e
🔍 Latest deploy log https://app.netlify.com/sites/docs-frontend-dotcomstg/deploys/6761d1c6cbab5400084ea38b
😎 Deploy Preview https://deploy-preview-1329--docs-frontend-dotcomstg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for docs-frontend-stg failed. Why did it fail? →

Name Link
🔨 Latest commit dd0ad5e
🔍 Latest deploy log https://app.netlify.com/sites/docs-frontend-stg/deploys/6761d1c6e1f1670008668e14

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for docs-frontend-stg failed. Why did it fail? →

Name Link
🔨 Latest commit 01d3bf8
🔍 Latest deploy log https://app.netlify.com/sites/docs-frontend-stg/deploys/6762fca5f0237a00085f64b6

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for docs-frontend-dotcomstg ready!

Name Link
🔨 Latest commit 01d3bf8
🔍 Latest deploy log https://app.netlify.com/sites/docs-frontend-dotcomstg/deploys/6762fca5aea2b900080bc936
😎 Deploy Preview https://deploy-preview-1329--docs-frontend-dotcomstg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@branberry branberry requested a review from rayangler December 20, 2024 17:23
@@ -1,4 +1,5 @@
const swc = require('@swc/core');
const { load } = require('js-toml');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove from here?

`;

function isSelectedTab(slug) {
if (typeof window === 'undefined') return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have an existing is-browser util. Is that something we can leverage here?

Comment on lines +14 to +23
const FormatTitle = styled.div`
margin-left: var(--margin-left);
scroll-margin-bottom: ${theme.size.xxlarge};
`;

const caretStyle = css`
margin-top: 3px;
margin-right: 5px;
min-width: 16px;
`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Nit) Going forward, we might want to consolidate to using LG's emotion package instead so we don't have so many different ways of adding css

// This prevents LG's SideNav component from being seen in its collapsed state on mobile
return (
<>
<Global />
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of the empty Global component here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I removed the mobile styling in this Global component. I didn't remove the rest of it, though, which I'll do right now! Thanks!

onClick={() => setIsOpen(!isOpen)}
>
<Icon className={cx(caretStyle)} glyph={iconType} fill={palette.gray.base} onClick={onCaretClick} />
<FormatTitle style={{ '--margin-left': '3px' }}>{label}</FormatTitle>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this style needed? I see margin-left: 3px already declared

@@ -0,0 +1,5 @@
export function getFeatureFlags() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been wanting a util like this but haven't actively worked with any myself recently to justify adding it. Love this!

// groups are for adding a static header, these can also be collapsible
if (group) {
return (
<SideNavGroup header={label} collapsible={collapsible} className={cx(sideNavItemTOCStyling({ level }))}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need to omit sideNavItemTOCStyling from the groups themselves or tweak it for groups, depending on the use case. Seems like this may be causing awkward padding

Screenshot 2024-12-20 at 1 17 34 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah! Thanks for pointing this out, I'm noticing the awkward padding as well. I haven't updated the PR description to reflect this, but I think I might forgo too much styling work as the final design is still very much so up in the air. The focus for this PR is to get the scaffolding in place to read the toc.toml, and then render the side nav with it

<SideNavItem
hideExternalIcon={true}
as={Link}
to={url}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on the current toctree example, would it make sense to check if url exists here? In the deploy preview link, clicking it effectively reloads the current page and it's difficult to see the hidden menu items

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.

2 participants