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

WIP: add glossary with some basic DVC terms #431

Merged
merged 12 commits into from
Jul 20, 2019
Merged

WIP: add glossary with some basic DVC terms #431

merged 12 commits into from
Jul 20, 2019

Conversation

algomaster99
Copy link
Contributor

@algomaster99 algomaster99 commented Jun 11, 2019

This PR just initializes the glossary. Suggestions for more terms and concepts, to be included here, are more than welcome!

Fixes #424

@shcheklein shcheklein temporarily deployed to dvc-org-pr-431 June 11, 2019 00:53 Inactive
@jorgeorpinel jorgeorpinel changed the title Add glossary with some basic DVC terminologies WIP: add glossary with some basic DVC terms Jun 11, 2019
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

@algomaster99 let's first discuss on Discord what exactly and how do we want this doc to be structured. How will we be aligning it with the Basic concepts ticket for the user guide.

@jorgeorpinel

This comment has been minimized.

@algomaster99

This comment has been minimized.

@algomaster99

This comment has been minimized.

@algomaster99 algomaster99 requested a review from shcheklein June 18, 2019 13:46
@algomaster99

This comment has been minimized.

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Looks great! Still some work has to be done. Fix Z-index + check the comments I left.

Also, it's not 100% clear how are we going to reconcile two separate files - one of them for longer description one for small snippets that have a link to the longer version?

@algomaster99
Copy link
Contributor Author

@shcheklein I think we can delete glossary.md and generate it using the json file instead.

@algomaster99
Copy link
Contributor Author

@jorgeorpinel @shcheklein
Here the changes I have made till now:
tooltip

  • The issue with z-index has been resolved by controlling the direction it pops in.
  • The enun (pronunciation) has been removed.
  • The content is rendered as markdown in the tooltip.

@algomaster99

This comment has been minimized.

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Please, address a few comments that are still not addressed.

Let's remove the glossary.md file for now altogether. If we have a JS we can reference in the documentation contributing guide it should be enough, especially if we use to show those tool tips.

  • There should be a section or a few sections called Basic Concepts in the user guide that is properly on a user level language explains some missing concepts. The glossary tooltips could point to these sections among other things.

@algomaster99

This comment has been minimized.

@algomaster99
Copy link
Contributor Author

@jorgeorpinel @shcheklein
tooltip

@shcheklein shcheklein temporarily deployed to dvc-org-pr-431 July 19, 2019 00:05 Inactive
@jorgeorpinel

This comment has been minimized.

src/Tooltip/index.js Outdated Show resolved Hide resolved
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Looks awesome! Glad that SO helped after all. Just a couple questions from me and @jorgeorpinel . Please take a look. Please run it on different browsers, make sure that we don't break mobile (even if we don't show tooltips there yet).

@shcheklein shcheklein temporarily deployed to dvc-org-pr-431 July 19, 2019 07:27 Inactive
@algomaster99
Copy link
Contributor Author

@jorgeorpinel @shcheklein
Yeah, you can upvote it. There is only one answer :)
Please review as well. I have made some changes.

@algomaster99 algomaster99 requested a review from shcheklein July 19, 2019 07:30
@algomaster99
Copy link
Contributor Author

@shcheklein It is currently not designed for the mobile view so it is breaking a little - like it can go off-screen, etc.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 19, 2019

@algomaster99

currently not designed for the mobile view so it is breaking a little

Did you ever open a separate issue to address the mobile view? Or add it to the existing future issue #461 please.

Nevermind, I just moved all the missing stuff from this PR to that issue.

@shcheklein
Copy link
Member

@algomaster99 one more comment left to address (specify that .dvc/cache is the default location) + can we disable it on mobile if it breaks?

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

See the last comment from me.

@algomaster99
Copy link
Contributor Author

@jorgeorpinel @shcheklein I have made the changes. You can look at it :)

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉

@shcheklein shcheklein merged commit 848467e into master Jul 20, 2019
@algomaster99 algomaster99 deleted the glossary branch July 20, 2019 16:29
@jorgeorpinel jorgeorpinel mentioned this pull request Aug 11, 2019
14 tasks
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.

create glossary
3 participants