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

Updates for dvc.org integration #130

Merged
merged 5 commits into from
Nov 22, 2022
Merged

Updates for dvc.org integration #130

merged 5 commits into from
Nov 22, 2022

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Nov 17, 2022

These are changes that made integration with dvc.org and the new version better/easier/possible.

Companion to iterative/dvc.org#4118, which depends on this being merged and shipped.

Verified

This commit was signed with the committer’s verified signature.
vjik Sergei Predvoditelev
@rogermparent rogermparent temporarily deployed to gatsby-theme-dvc-integr-ejirjg November 17, 2022 01:12 Inactive
@rogermparent rogermparent changed the title Dvc integration updates Updates for dvc.org integration Nov 17, 2022
@rogermparent rogermparent force-pushed the dvc-integration-updates branch from 633d854 to 864bcb3 Compare November 18, 2022 02:27
@rogermparent rogermparent temporarily deployed to gatsby-theme-dvc-integr-a0fkxh November 18, 2022 02:27 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-dvc-integr-a0fkxh November 18, 2022 19:50 Inactive
@rogermparent rogermparent temporarily deployed to gatsby-theme-dvc-integr-a0fkxh November 18, 2022 20:13 Inactive
Comment on lines 11 to 29
<div
className={cn(
styles.alert,
'w-full',
'transition-all',
'ease-in-out',
'delay-150',
collapsed ? ['h-0'] : ['h-7']
'whitespace-nowrap',
'text-center',
'truncate',
'overflow-hidden',
'px-2',
collapsed ? 'h-0' : 'h-7'
)}
>
<span role="img" aria-label="rocket">
🚀
</span>{' '}
<Link href="https://studio.iterative.ai">DVC Studio</Link>, the online UI
for DVC, is live!{' '}
<span className="align-middle">
<AlertContent />
</span>
</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tweaked Alert component and extracted the content specifically to its own component. We can update it here to populate to all sites on update, or shadow to override per-site.

Comment on lines +1 to +20
import React from 'react'

import Link from '../../Link'
import { ReactComponent as LogoSVG } from '../../../images/dvc_icon-color--square_vector.svg'
import * as styles from './styles.module.css'

export const HeaderBranding = () => (
<>
<Link href="/" className={styles.logoLink} title="DVC" aria-label="DVC">
<LogoSVG className={styles.logo} />
</Link>
<Link
className={styles.company}
href="https://iterative.ai/"
target="_blank"
>
by <span className={styles.companyName}>iterative.ai</span>
</Link>
</>
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored branding into its own component so we can override it per-site more easily without shadowing the whole header.

Comment on lines 14 to +15

import navLinkItemsData from './data'
import navLinkItemsData from '../../../../data/headerNav'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This data file is very different between this example site and dvc.org. Slightly excessive shadowing paves over it, but we'll need something better soon (we have a ticket for something like this).

}

export interface INavLinkPopupData extends INavLinkCommonData {
popup: PopupName
text: string | ReactNode
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type fix, some pieces of data have a React component as its text.

@@ -4,6 +4,7 @@ import cn from 'classnames'

import 'reset-css'
import './base.css'
import './fonts.css'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allows us to shadow specifically this fonts file without needing to clobber the rest of base.css.

Comment on lines -1 to 8
import { getFirstPage } from '../../../../utils/shared/sidebar'
import { INavLinkData, INavLinkPopupData } from './types'
import { getFirstPage } from '../utils/shared/sidebar'
import {
INavLinkData,
INavLinkPopupData
} from '../components/LayoutHeader/Nav/LinkItems/types'

const docsPage = getFirstPage()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought this would be a better place for this data file, but could and probably will be improved.

@rogermparent rogermparent marked this pull request as ready for review November 18, 2022 20:19
@rogermparent rogermparent requested a review from a team as a code owner November 18, 2022 20:19
Copy link
Contributor

@yathomasi yathomasi left a comment

Choose a reason for hiding this comment

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

Looks good to me. Since we are working on the nav section with a separate data file, should we also update the Hamburger menu to use the data?

@rogermparent rogermparent self-assigned this Nov 21, 2022
@rogermparent
Copy link
Contributor Author

Looks good to me. Since we are working on the nav section with a separate data file, should we also update the Hamburger menu to use the data?

Good idea! We absolutely need a better nav system, but I decided to deliberately hold myself back from bloating this PR with a big nav overhaul that would make it take longer. We can discuss more during the planning meeting.

@rogermparent rogermparent merged commit ec9a925 into main Nov 22, 2022
@rogermparent rogermparent deleted the dvc-integration-updates branch November 22, 2022 15:10
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.

None yet

2 participants