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

Add doc/user-guide/glossary page #2595

Merged
merged 8 commits into from
Jul 7, 2021
Merged

Add doc/user-guide/glossary page #2595

merged 8 commits into from
Jul 7, 2021

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Jun 30, 2021

  • creates a template called doc-jsx.jsx for creating documentation pages with jsx instead of markdown
  • creates a doc/user-guide/glossary page that lists all tooltip data in this format:
# Glossary

<b id="bold-term">Bold term</b>: description text

Fixes #1395

@jorgeorpinel @casperdcl @iesahin

@julieg18 julieg18 marked this pull request as draft June 30, 2021 15:51
@shcheklein shcheklein temporarily deployed to dvc-org-add-glossary-pa-jxqnju June 30, 2021 15:51 Inactive
@julieg18 julieg18 changed the title Add glossary page Add doc/user-guide/glossary page Jun 30, 2021
@julieg18 julieg18 force-pushed the add-glossary-page branch from d58df78 to 3804c90 Compare June 30, 2021 16:04
@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju June 30, 2021 16:05 Inactive
{
"label": "What is DVC?",
"slug": "what-is-dvc"
},
{
"slug": "project-structure",
"source": "user-guide/project-structure/index.md",
"source": "project-structure/index.md",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that this page and the page on line 143 had incorrect source causing the "Edit on Github" link to take you to a 404 page. I went ahead and fixed them.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 2, 2021

Choose a reason for hiding this comment

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

Interesting. Should the engine throw an error when building? Out of scope for this PR but could open an issue (or move the fix to another PR and we can decide there). Same for line 141

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about the engine throwing an error, but I don't mind moving these fixes to a separate PR if needed!

@julieg18 julieg18 marked this pull request as ready for review June 30, 2021 16:16
@julieg18 julieg18 requested a review from rogermparent June 30, 2021 16:16
content/docs/sidebar.json Outdated Show resolved Hide resolved
Comment on lines 97 to 101
{
"label": "Glossary",
"slug": "glossary",
"source": "basic-concepts"
},
Copy link
Contributor

@casperdcl casperdcl Jun 30, 2021

Choose a reason for hiding this comment

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

needs to be moved further down the sidebar, question is where? CC @iterative/docs

Personally I think "What is DVC?" and "Project Structure" are closely related to "Glossary" (compared to any other sidebar item) but don't know if that's enough justification to keep them near each other.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, that we should move it down. Probably even to the bottom of the UG?

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 Glossary can be the last section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved glossary to the bottom of user-guide for now.

@casperdcl casperdcl added A: docs Area: user documentation (gatsby-theme-iterative) website: eng-doc DEPRECATED JS engine for /doc type: enhancement Something is not clear, small updates, improvement suggestions labels Jun 30, 2021
@julieg18 julieg18 force-pushed the add-glossary-page branch from 3804c90 to a3364a4 Compare June 30, 2021 20:34
@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju June 30, 2021 20:34 Inactive
@julieg18 julieg18 force-pushed the add-glossary-page branch from a3364a4 to 2d840b0 Compare June 30, 2021 21:18
@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju June 30, 2021 21:19 Inactive
* add Documentation Markdown/Main and WithJSX components
* add template for creating doc pages with jsx instead of markdown
* add glossary page to pages directory
@julieg18 julieg18 force-pushed the add-glossary-page branch from 2d840b0 to ca9c4fa Compare June 30, 2021 22:01
@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju June 30, 2021 22:01 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.

Looks good on the code end! I'll just give this a comment for now and let the docs team approve when the sidebar and any other requests are handled.

Nice work!

@casperdcl casperdcl requested a review from a team July 1, 2021 00:42
@shcheklein
Copy link
Member

One thing - let's try to generate 3rd or 2nd level titles? This way items will have links? @iterative/docs WDYT?

@iesahin
Copy link
Contributor

iesahin commented Jul 1, 2021

Nice work @julieg18 Thank you.

If I open the glossary link directly, it's like this:
Screen Shot 2021-07-01 at 11 33 10

Link is: https://dvc-org-add-glossary-pa-jxqnju.herokuapp.com/doc/user-guide/glossary

if I navigate the page from the sidebar, it's like this:
Screen Shot 2021-07-01 at 11 33 32

This is with Firefox 89 on macOS 11.4

In the first version, there is a sudden collapse after loading the page.

@casperdcl
Copy link
Contributor

casperdcl commented Jul 1, 2021

let's try to generate 3rd or 2nd level titles? This way items will have links?

All entries already have #ids but I guess you mean there should be an on-hover link icon appear on the left of entries.

One work-around may be to make them titles indeed, but of a new class where they appear inline (no need to separate title & description with a newline)

@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju July 1, 2021 12:21 Inactive
@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju July 1, 2021 12:46 Inactive
@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju July 1, 2021 13:20 Inactive
@julieg18
Copy link
Contributor Author

julieg18 commented Jul 1, 2021

Hyperlink is now added to heading(and can be added to any other headings on the page if we decide to do so) and all the glossary elements should be rendering properly!

@casperdcl
Copy link
Contributor

and can be added to any other headings on the page if we decide to do so

yes please (as in hovering on a term will show a link icon 🔗 on the left of it)

@julieg18
Copy link
Contributor Author

julieg18 commented Jul 1, 2021

yes please (as in hovering on a term will show a link icon 🔗 on the left of it)

Do we want to convert the terms' names into h2s or try to mimic a headers link behavior while keeping the terms how they look now?

@casperdcl
Copy link
Contributor

The "look" should be the same as now. Whether internally they are somehow h2s without line breaks or (probably easier) mimic link behaviour is up to you.

@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju July 1, 2021 22:06 Inactive
@julieg18
Copy link
Contributor Author

julieg18 commented Jul 1, 2021

The terms' names are now hyperlinks, acting like Header hyperlinks!

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

awesome

src/components/Documentation/index.tsx Show resolved Hide resolved
@jorgeorpinel
Copy link
Contributor

I never noticed the Codeclimate check failing in docs repo PRs. Is it an issue?

Comment on lines +177 to 181
{
"label": "Glossary",
"slug": "glossary",
"source": "basic-concepts"
}
Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 2, 2021

Choose a reason for hiding this comment

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

What would it take to reduce this sidebar entry to just "glossary"? Moving the basic-concepts/ folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I'm understanding your question 🤔 Are you referring to deleting source or renaming the source value to glossary? The source is used to build the "Edit on Github" button. If you're asking about deleting the source, we don't have a /content/docs/user-guide/glossary.md so deleting the source would end up creating a 404 link. I linked the basic-concepts folder, since that is where we're currently holding all of our tooltip data. If you're asking about renaming the source to glossary, I suppose we could do that by renaming the basic-concepts folder to glossary 😃

Copy link
Contributor

@casperdcl casperdcl Jul 2, 2021

Choose a reason for hiding this comment

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

What would it take to reduce this sidebar entry to just "glossary"? Moving the basic-concepts/ folder?

I think by "sidebar entry" you meant page URL slug?

The sidebar entry is "Glossary," and the page URL slug is "glossary." The "source" folder is only used for the "Edit on GH" button.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 7, 2021

Choose a reason for hiding this comment

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

reduce this sidebar entry to just "glossary"

I was referring to this:

Suggested change
{
"label": "Glossary",
"slug": "glossary",
"source": "basic-concepts"
}
"glossary"

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 7, 2021

Choose a reason for hiding this comment

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

If you're asking about renaming the source to glossary, I suppose we could do that by renaming the basic-concepts folder to glossary

Right. Should we do that? But again... #550 (see #2595 (review) below).

Copy link
Contributor Author

@julieg18 julieg18 Jul 7, 2021

Choose a reason for hiding this comment

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

I don't think we'd be able to do that(replacing the whole object with "glossary") since the "Edit on Github" link would default to https://github.com/iterative/dvc.org/blob/master/content/docs/user-guide/glossary.md which doesn't exist.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 10, 2021

Choose a reason for hiding this comment

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

I just noticed this. Hmm so it's a special/hardcoded nav entry then. Not sure I love that but oh well, this is merged!

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait no I see. I think it could default to /content/docs/user-guide/glossary/index.md too? Again, this is merged so np but I was thinking what does it take to decouple it completely from basic-concepts/

Copy link
Contributor

Choose a reason for hiding this comment

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

confusion related to #2595 (comment)

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

https://dvc-org-add-glossary-pa-jxqnju.herokuapp.com/doc/user-guide/glossary looks is great, thanks @julieg18! I'm just unsure where should it go in the nav but that's easy to change later.

I'm slightly worried that we won't have a good index for the actual Basic Concepts section (#550) per #1395 (comment). Are we sure we're not going to want to move this into there? Because that would require a permanent redirect at that point. Cc @casperdcl @shcheklein @iesahin

Other than that, I'm curious about the significant code changes but I'll leave that for @rogermparent perhaps since I'm unable to determine exactly how they relate to the Glossary page. Specifically:

  • src/components/Documentation/Markdown/inde/Main/ folder was created with 2 pre-existing files but unfortunately GH didn't pick up the move, so it's hard to tell what changed.
  • What is src/components/Documentation/WithJSX/AutoLinkElement/ ? (index.tsx:67 looks fun)
  • Same for src/templates/doc-jsx.tsx . Where is IJSXDocPageProps used?

Thanks

@julieg18
Copy link
Contributor Author

julieg18 commented Jul 2, 2021

  • src/components/Documentation/Markdown/inde/Main/ folder was created with 2 pre-existing files but unfortunately GH didn't pick up the move, so it's hard to tell what changed.

@jorgeorpinel Markdown/index.tsx originally took in htmlAst as props and rendered most of the documentation page. We needed to be able render that part of the documentation page with JSX or htmlAst so I took the majority of Markdown/index.tsx and placed it in a separate component called Markdown/Main. The Main component is used to render part of documentation page whether you want to render JSX or htmlAST.

  • What is src/components/Documentation/WithJSX/AutoLinkElement/ ? (index.tsx:67 looks fun)

We have a gatsby-plugin that converted our markdown headings into hyper-links. To render hyper-links with JSX, I created the AutoLinkElement. It already has access to the needed css, so all I needed to do was create the html and add the right classes.

  • Same for src/templates/doc-jsx.tsx . Where is IJSXDocPageProps used?

Whoops, that's a typo. I'll delete the export. Thanks for spotting that! As for the template itself, it basically renders the same as the doc.tsx template, except it takes JSX instead of HTMLAst.

I never noticed the Codeclimate check failing in docs repo PRs. Is it an issue?

I don't think so. Looking at the CodeClimate review, correcting any of the issues mentioned would only create unnecessarily complex code. But if you think something could be better with the code, feel free to mention it!

@julieg18 julieg18 temporarily deployed to dvc-org-add-glossary-pa-jxqnju July 2, 2021 12:27 Inactive
@casperdcl
Copy link
Contributor

Are we sure we're not going to want to move this into there? Because that would require a permanent redirect at that point

I don't understand. Is this related to this confusion?

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 7, 2021

Thanks for the explanations @julieg18! Seems like the code has been peer reviewed so 👌

Are we sure we're not going to want to move this into there? Because that would require a permanent redirect at that point

I don't understand. Is this related to this confusion?

It's related to the naming somewhat, yes @casperdcl. But there's a reason we have glossary tests in basic-concepts/ right? They're closely related. So what will we have as index for Basic Concetps? I know it's beyond the scope here technically but not addressing it now may mean having to do a redirect later, which we can do (I'm not trying to block this merge) but its not ideal.

I'll mention it in tomorrow's meeting.

@julieg18
Copy link
Contributor Author

julieg18 commented Jul 7, 2021

Are we good to merge this, or is there more to discuss?

@casperdcl
Copy link
Contributor

I still don't know what @jorgeorpinel means by "redirect" (i.e. redirect what to what?) but I suspect by "redirect" he is referring to people bookmarking the "edit on GH" URL rather than any dvc.org URL.

  1. Not something anyone does
  2. Not something we need to care about
  3. The source folder layouts are absolutely irrelevant, the only thing which matters is dvc.org URLs never changing or redirecting.

I also don't know what @jorgeorpinel means by "index" but I suspect that once again he means where in source code does a different issue (#550) get addressed, and the answer as repeatedly stated in both issues & meetings is in the same folder. The yml/md source files each will contain 2 keys (short & long descriptions: the short ones are rendered to Glossary, the long ones to BC #550).

Definite immediate merge to me unless @jorgeorpinel is envisioning a different source layout.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 7, 2021

I'm not blocking the merge.

@casperdcl you're a bit confused:

By redirect I mean https://github.com/iterative/dvc.org/blob/master/redirects-list.json i.e. an actual HTTP 301/303 redirect.

dvc.org URLs never changing or redirecting

Exactly. I'm trying to prevent it from changing and redirecting if we have to move the glossary/ to basic-concepts/ (index) soon after this is merged. Like I said, I'll bring it up in the next meeting...

By index I mean index.md, which is shown for parent nav entries e.g. https://dvc.org/doc/start (content/doc/start/index.md)

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 7, 2021

UPDATE: OK, we discussed this in a meeting (cc @shcheklein and @iesahin) and we agreed to keep the glossary page where it is since it may take a while to get to #550 anyway. Also we can use it initially as the "Basic Concepts index" if we only have a few BCs (e.g. #2453) by linking them from this glossary page. If it needs a redirect in the future, so be it.

@jorgeorpinel jorgeorpinel merged commit 353457f into master Jul 7, 2021
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 7, 2021

Thanks again @julieg18 ! 🎉

@jorgeorpinel jorgeorpinel mentioned this pull request Jul 8, 2021
1 task
@casperdcl casperdcl deleted the add-glossary-page branch July 11, 2021 10:20
@casperdcl
Copy link
Contributor

By redirect I mean https://github.com/iterative/dvc.org/blob/master/redirects-list.json i.e. an actual HTTP 301/303 redirect.

But neither glossary nor basic-concepts currently and in future will ever appear in that file?

dvc.org URLs never changing or redirecting

Exactly. I'm trying to prevent it from changing and redirecting if we have to move the glossary/ to basic-concepts/ (index) soon after this is merged.

completely lost

By index I mean index.md, which is shown for parent nav entries e.g. dvc.org/doc/start (content/doc/start/index.md)

#2595 (comment):

Wait no I see. I think it could default to /content/docs/user-guide/glossary/index.md too? Again, this is merged so np but I was thinking what does it take to decouple it completely from basic-concepts/

  • There is no such thing as /content/docs/user-guide/glossary/index.md.
  • There is no such thing as /content/docs/user-guide/glossary/.
  • There is no such thing as /content/docs/user-guide/basic-concepts/index.md.

And none of the above 3 things will ever exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: docs Area: user documentation (gatsby-theme-iterative) type: enhancement Something is not clear, small updates, improvement suggestions website: eng-doc DEPRECATED JS engine for /doc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glossary: create Glossary page (besides Basic Concepts?)
6 participants