Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Add docs engine #45

Merged
merged 36 commits into from
May 20, 2022
Merged

Add docs engine #45

merged 36 commits into from
May 20, 2022

Conversation

rogermparent
Copy link
Contributor

Extends #43

Adds @dvcorg/gatsby-theme-iterative, which includes all the code and assets of dvc.org's blog engine, along with a lot of opinions from dvc.org that break the site a bit.

@rogermparent rogermparent changed the base branch from main to docs-content April 23, 2022 16:17
},
'gatsby-transformer-sharp',
'gatsby-plugin-sharp',
'gatsby-plugin-preact',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preact breaks with React 18 installed, since Gatsby attempts to use the renderToPipeableStream API that Preact does not have.

Ways to solve are:

  1. Remove Preact plugin
  2. Downgrade to React 17

I've opted for the first for now, I'll try the second and contrast the resulting sites in Lighthouse to see which is more performant between Preact and React 18.

Comment on lines +5 to +11
content: [
'./src/**/*.js',
'./src/**/*.jsx',
'./src/**/*.ts',
'./src/**/*.tsx'
],
darkMode: 'media',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tailwind schema changed in an update

@@ -7,39 +7,15 @@ module.exports = {
? `https://${process.env.HEROKU_APP_NAME}.herokuapp.com`
: 'https://mlem.ai',
twitterUsername: '@DVCorg',
titleTemplate: '%s | MLEM'
titleTemplate: '%s | MLEM',
keywords: ['mlem']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

theme requires keywords, we probably will want better ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Studies show that keywords don't realy make a difference in the site. We can probably just add a couple like mlem, ml, and ml deployment.

Comment on lines 20 to 21
resolve: 'gatsby-plugin-manifest',
options: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the theme to this site reminded me that manifest is going to be pretty unique across even sites with the same theme, so I've removed it from the theme and it must be manually specified.

We can probably find a more streamlined way to handle the manifest and ensure it mandates all the fields we want (favicons come to mind)

@rogermparent rogermparent reopened this Apr 25, 2022
@rogermparent rogermparent marked this pull request as ready for review April 25, 2022 19:22
@rogermparent rogermparent reopened this Apr 25, 2022
@rogermparent rogermparent changed the base branch from docs-content to main April 25, 2022 19:24
@rogermparent rogermparent reopened this Apr 25, 2022
@rogermparent
Copy link
Contributor Author

Sorry for any notification spam that may have happened here, I was trying to get Gatsby Cloud to catch onto this pre-existing PR.

Lessons learned:

  • The PR must be targeted onto master to be detected by the build system
  • Reopening and re-closing works
  • I toggled draft off, but that didn't work without re-targeting the PR

@gatsby-cloud
Copy link

gatsby-cloud bot commented Apr 25, 2022

Gatsby Cloud Build Report

mlem.ai

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 1m

Performance

Lighthouse report

Metric Score
Performance 💚 95
Accessibility 🔶 88
Best Practices 💚 100
SEO 🔶 83

🔗 View full report

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great job so far! The docs have definitely come a long way since the pr was first created. Minus a couple bugs I spotted, everything else looks to be working correctly!

src/components/Footer/index.tsx Show resolved Hide resolved
@@ -0,0 +1,224 @@
import React, { useState, useEffect, useRef } from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

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

The border line in the content list is missing some margin:

image

@rogermparent
Copy link
Contributor Author

Alright, we have lint checks and pass them, time to clean up the last few issues from @julieg18's review and we should be good to go!

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented May 18, 2022

Question: where is the docs engine repo? Maybe we should move some questions, issues, etc. to it. E.g. #59 (comment) or #43 (comment).

@rogermparent
Copy link
Contributor Author

where is the docs engine repo?

https://github.com/iterative/gatsby-theme-iterative, though many changes will be done in this PR and backported in the interest of speed.

@jorgeorpinel
Copy link
Contributor

I get a 404.

@rogermparent
Copy link
Contributor Author

Sorry about that, I forgot that access was limited! I've added read permissions for the docs team on the theme repo, the same link should work now.

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Found a couple of other small bugs and had a question about the CSS.

@@ -1,3 +1,4 @@
@custom-media --sm-scr (min-width: 414px);
@custom-media --xs-scr (max-width: 572px);
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding things correctly, the reason the mobile styles we're breaking was because the CSS in our docs theme relies on this media.css?

If that's the case, shouldn't the CSS in the docs theme either use it's own media.css or just not have one at all if it's easier to impliment. The media.css that the docs theme normally relies on is very different from ours:

    '--xxs-scr': '(max-width: 376px)',
    '--xs-scr': '(max-width: 572px)',
    '--sm-scr': '(max-width: 768px)',
    '--md-scr': '(max-width: 1004px)',
    '--lg-scr': '(min-width: 1005px)',
    '--xl-scr': '(min-width: 1200px)'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct, this set of media queries was made as a compromise that fixes the breaks between the proper mobile-first tailwind-based media queries on mlem.ai and the postcss-custom-media queries from dvc.org that are mixed between max-width and min-width. The docs theme does need media queries since the docs themselves depend on them, but we definitely can use a more elegant implementation.

@@ -0,0 +1,224 @@
import React, { useState, useEffect, useRef } from 'react'
import cn from 'classnames'
Copy link
Contributor

Choose a reason for hiding this comment

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

The github button isn't staying at the top of the page:

image

}}
/>
</div>
<div className={styles.content}>
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, the left sidebar is spliing into the content:

image

@julieg18
Copy link
Contributor

julieg18 commented May 20, 2022

Sorry about that, I forgot that access was limited! I've added read permissions for the docs team on the theme repo, the same link should work now.

Shouldn't the entire company have read or even write permissions by default?

@rogermparent
Copy link
Contributor Author

rogermparent commented May 20, 2022

Shouldn't the entire company have read or even write permissions by default?

Not sure if that's the intention, but if it is then that wasn't the case for this one- I had to add the Websites team a while ago in similar fashion to the docs team now. I vaguely remember something about SOC2 compliance leading us in the direction of keeping private repos accessible only to those that have a reason to access them, so that could explain why the defaults are how they are if it's purposeful.

If we want to give everyone in the organization access by default, I believe a "Base Role" has to be added. I can see there's no Base Role on this repo, but if we want to set one we'll need an Organization Owner to do so.
image

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

While we do have some bugs that need to get taken care of, since the engine is basically done, we should be good to merge as a first iteration!

@@ -0,0 +1,38 @@
# MLEM Documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

We could keep this page as is or maybe put a note somewhere that the rest of the docs are coming soon 🤔

@rogermparent
Copy link
Contributor Author

While there's still a few styling issues, we're pretty sure all the showstoppers have been handed so we've decided to merge this PR into mlem.ai and take care of the rest in follow-ups. This should make it easier to make more normal PRs both for us and @iterative/docs instead of keeping up the massive long-lived PRs like this and #43.

@julieg18
Copy link
Contributor

If we want to give everyone in the organization access by default, I believe a "Base Role" has to be added. I can see there's no Base Role on this repo, but if we want to set one we'll need an Organization Owner to do so.

You can use our @"iterative/all" (added double quotes so I don't actually ping everybody 😅) to give everyone access. This is how it looks in mlem.ai:
image

@rogermparent
Copy link
Contributor Author

I hadn't thought of that! I wasn't able to see other repo's permissions so I wasn't sure what the standard is.
Here's what the theme repo looks like now:
image
Thanks @julieg18!

@rogermparent rogermparent merged commit 8c0258e into main May 20, 2022
@rogermparent rogermparent deleted the docs-engine branch May 20, 2022 18:10
@rogermparent rogermparent mentioned this pull request May 20, 2022
@rogermparent rogermparent mentioned this pull request May 25, 2022
@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) A: website Website development labels Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A: docs Area: user documentation (gatsby-theme-iterative) A: website Website development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants