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

Update theme to 0.2.x #402

Merged
merged 22 commits into from
Dec 16, 2022
Merged

Update theme to 0.2.x #402

merged 22 commits into from
Dec 16, 2022

Conversation

yathomasi
Copy link
Contributor

@yathomasi yathomasi commented Nov 28, 2022

Similar to iterative/dvc.org#4118 for cml.dev.

@yathomasi yathomasi self-assigned this Nov 28, 2022
@shcheklein shcheklein had a problem deploying to cml-dev-update-theme-xdnnu7hzr November 28, 2022 15:36 Failure
@github-actions
Copy link

github-actions bot commented Nov 28, 2022

3e3aadf

Link Check Report

There were no links to check!

@yathomasi yathomasi had a problem deploying to cml-dev-update-theme-z122jto7y December 1, 2022 11:06 Failure
@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-z122jto7y December 1, 2022 11:07 Inactive
@shcheklein shcheklein temporarily deployed to cml-dev-update-theme-i7rajgaw2 December 1, 2022 11:22 Inactive
@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-i7rajgaw2 December 1, 2022 16:15 Inactive
@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-i7rajgaw2 December 1, 2022 17:44 Inactive
@yathomasi yathomasi marked this pull request as ready for review December 1, 2022 17:53
@yathomasi yathomasi requested a review from a team as a code owner December 1, 2022 17:53
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Looking good so far! Found a lot of small things though:

  • Homepage GitHub star icon aren't aligned

  • Homepage links don't have same style (underline)
    image

  • Home feature icons misaligned
    image

  • MLOps ecosystem text not bold
    image
    original:
    image

  • Docs fonts are different
    image
    original:
    image

Comment on lines +1 to +72
import React, { useEffect, useState } from 'react'
import Promise from 'promise-polyfill'
import { loadResource } from '@dvcorg/gatsby-theme-iterative/src/utils/front/resources'

import * as styles from '@dvcorg/gatsby-theme-iterative/src/components/Documentation/Layout/SearchForm/styles.module.css'

declare global {
// eslint-disable-next-line @typescript-eslint/naming-convention
interface Window {
docsearch?: (opts: object) => void
}
}

const apiKey = '3e17d424c7a90fede27b848fb01c0dc2'
const appId = '1O03WAGL0D'
const indexName = 'cml'

const SearchForm: React.FC = props => {
const [searchAvailable, setSearchAvailable] = useState<boolean>(false)
useEffect(() => {
console.log({ window, docSearch: window?.docsearch })
if (window) {
if (!window.docsearch) {
Promise.all([
loadResource(
'https://cdn.jsdelivr.net/npm/[email protected]/dist/cdn/docsearch.min.css'
),
loadResource(
'https://cdn.jsdelivr.net/npm/[email protected]/dist/cdn/docsearch.min.js'
)
]).then(() => {
if (window.docsearch) {
window.docsearch({
appId,
apiKey,
indexName,
inputSelector: '#doc-search',
debug: false // Set to `true` if you want to inspect the dropdown
})
setSearchAvailable(true)
}
})
} else {
window.docsearch({
appId,
apiKey,
indexName,
inputSelector: '#doc-search',
debug: false // Set to `true` if you want to inspect the dropdown
})
setSearchAvailable(true)
}
}
}, [])

return (
<div className={styles.searchArea}>
<div className={styles.container}>
<input
className={styles.input}
type="text"
id="doc-search"
placeholder="Search docs"
disabled={!searchAvailable}
{...props}
/>
</div>
</div>
)
}

export default SearchForm
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to follow up on making this more reusable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have had this pending for a while

Comment on lines 1 to 5
import React from 'react'
import SmartLink from '../../../atoms/SmartLink'
import SmartLink from '../../../../../components/atoms/SmartLink'
import * as styles from './styles.module.css'

const Alert: React.FC = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to drop the Alert component and let the theme control it now.

Comment on lines 1 to 7
import React from 'react'

import ModesProvider from './src/components/organisms/SwitchableMode/Provider'

export const wrapRootElement = ({ element }) => (
<ModesProvider>{element}</ModesProvider>
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be better to just wrap our components in the normal React area in this rather than use the wrapRootElement escape hatch.

@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-nsg1abwas December 13, 2022 09:31 Inactive
@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-nsg1abwas December 13, 2022 10:09 Inactive
@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-nsg1abwas December 13, 2022 11:09 Inactive
@rogermparent
Copy link
Contributor

image
Looks like we'll have to shadow some colors and/or improve the theme-changing support. I think we can simply shadow the CSS file for minimal duplication, but we can at least use the theme's content component even if we have to replace the wrapper.

@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-nsg1abwas December 14, 2022 17:35 Inactive
@yathomasi
Copy link
Contributor Author

Looks like we'll have to shadow some colors and/or improve the theme-changing support. I think we can simply shadow the CSS file for minimal duplication, but we can at least use the theme's content component even if we have to replace the wrapper.

I just used the same approach 😄. It's working fine locally and let's see how it goes to preview deployment as the previous one was also fine locally but not on preview deployment.

@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-nsg1abwas December 14, 2022 17:57 Inactive
@rogermparent
Copy link
Contributor

Looking great! I've been clicking around and can only see one final thing wrong: we need scroll-margin-top/scroll-padding-top for anchor links so clicking internal links works properly.

After that, I think we're ready for approval!

2022-12-14.22-50-15.mp4

@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-nsg1abwas December 15, 2022 06:31 Inactive
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Fantastic work @yathomasi! The scroll margin issue is fixed enough, and now I can't find any showstoppers browsing around the deploy or looking through the code.

@@ -15,7 +15,7 @@ import {
} from "../../../../gatsby-plugin-theme-ui/components"

const UseCasesSection: React.ForwardRefRenderFunction<HTMLElement> = () => (
<section id="use-cases">
<section id="use-cases" style={{scrollMarginTop:"-3.5rem"}} >
Copy link
Contributor

Choose a reason for hiding this comment

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

might want to do this with tailwind but not a huge deal right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't do well with the tailwind so I had to increase specificity with inline CSS.

@yathomasi yathomasi temporarily deployed to cml-dev-update-theme-nsg1abwas December 16, 2022 11:25 Inactive
@yathomasi yathomasi merged commit 3e3aadf into master Dec 16, 2022
@yathomasi yathomasi deleted the update-theme branch December 16, 2022 12:45
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.

3 participants