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

feat: adds glossary #211

Closed
wants to merge 16 commits into from
Closed

feat: adds glossary #211

wants to merge 16 commits into from

Conversation

Lymah123
Copy link
Contributor

@Lymah123 Lymah123 commented Nov 20, 2023

Description

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Added to documentation?

  • 📜 README.md
  • 📓 docs.opensauced.pizza
  • 🍕 dev.to/opensauced
  • 📕 storybook
  • 🙅 no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Copy link

netlify bot commented Nov 20, 2023

👷 Deploy request for docs-open pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 16b7732

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks

Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.

Issue Reference

In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our Contributing Guide.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.

Protected Branch

In order to be considered for merging, the pull request changes must not be implemented on the "main" branch. This is described in our Contributing Guide. We would suggest that you close this PR and implement your changes as described in our Contributing Guide and open a new pull request.

@Lymah123
Copy link
Contributor Author

Hi @BekahHW and @CBID2 , I fixed the conflicting files with this PR.

@Lymah123 Lymah123 changed the title bug : fixes merge conflict fix: fixes merge conflict Nov 20, 2023
@Lymah123 Lymah123 changed the title fix: fixes merge conflict feat: adds glossary Nov 20, 2023
@Lymah123
Copy link
Contributor Author

Hi, @BekahHW and @CBID2, you can now review. I will be waiting!

Thank you.

@CBID2
Copy link
Contributor

CBID2 commented Nov 21, 2023

Hi @BekahHW and @CBID2 , I fixed the conflicting files with this PR.

Hi @Lymah123. Can you add the issue number to the `Related Documents and Tickets" section instead of the Description? It'll make it easier for the PR and issue to close.

Copy link
Contributor

@CBID2 CBID2 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 @Lymah123! :)

@CBID2 CBID2 added the 💡 feature A label to note if work is a feature label Nov 21, 2023
@Lymah123 Lymah123 changed the title feat: adds glossary feat: adds glossary Nov 21, 2023
@Lymah123
Copy link
Contributor Author

Hi @BekahHW and @CBID2 , I fixed the conflicting files with this PR.

Hi @Lymah123. Can you add the issue number to the `Related Documents and Tickets" section instead of the Description? It'll make it easier for the PR and issue to close.

Hi @CBID2, how do you mean? I didn't get that.

@adiati98
Copy link
Member

@Lymah123 you added a setting-up-a-new-repository.md here. I don't think that it is part of the newest Maintainer section.

@BekahHW can you confirm this?

@@ -11,6 +11,11 @@ Pretty often when opening a pull request it is very likely to run into merge con
To better illustrate the commands listed here at will use commits and screenshots from [open-sauced#1078](https://github.com/open-sauced/open-sauced/pull/1078).


## Repository setup
Copy link
Member

@adiati98 adiati98 Nov 21, 2023

Choose a reason for hiding this comment

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

@Lymah123 There is a duplication of this title in line 19. Which one do you want to keep?

Also, let's use capital letters for titles. :)

Suggested change
## Repository setup
## Repository Setup

Copy link
Member

@adiati98 adiati98 left a comment

Choose a reason for hiding this comment

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

Hi @Lymah123,
I've left some feedback for you here. :)

Also, I pulled your branch and noticed that you worked on your changes on the main branch.
Please read the PR Compliance Check in the "Protected Branch" section.

Therefore, as per PR Complience Check, I suggest that you:

  • Create a new branch and make your changes there.
  • Close this PR and create a new PR.

@@ -0,0 +1,33 @@
# OpenSauced Glossary
Copy link
Member

Choose a reason for hiding this comment

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

The heading 1 comes from the data. So let's add the data here and not a heading:

Suggested change
# OpenSauced Glossary
---
id: glossary
title: 'OpenSauced Glossary'
sidebar_label: 'OpenSauced Glossary'
keywords:
- glossary
---

Comment on lines +4 to +5
## PR Velocity
The PR Velocity measures the percentage and amount of days that pull requests that are merged. It is a great way to track the progress of your open source contributions.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a space between the title and paragraph here.
Besides that it is the best practice, we will do this going forward for consistency.

Suggested change
## PR Velocity
The PR Velocity measures the percentage and amount of days that pull requests that are merged. It is a great way to track the progress of your open source contributions.
## PR Velocity
The PR Velocity measures the percentage and amount of days that pull requests that are merged. It is a great way to track the progress of your open source contributions.

@Lymah123
Copy link
Contributor Author

@Lymah123 you added a setting-up-a-new-repository.md here. I don't think that it is part of the newest Maintainer section.

@BekahHW can you confirm this?

Hi @adiati98 , I didn't change anything from that file. I only touched it due to merge conflicts I had.

@adiati98
Copy link
Member

@Lymah123 in that case, I believe it was one of the files from previous version of Maintainers Guide before @BekahHW's newest version.

And because you made changes directly in the main branch and need to create new branch, let's close this PR and start all fresh! 🙂

The main branch of your forked repository also contains your latest changes, which we need to remove.

lymah main branch

Steps

Here's what you can do:

  1. Delete your forked repository. --> deleting your forked repository won't affect the original repository.
  2. Delete your local repository.
  3. Fork the docs repository.
  4. Clone the repository locally.
  5. Create a new branch where you will work on your changes.
  6. Create a glossary.md in the community folder and copy-paste the content from the file in this PR. Please also refer to the feedback reviews for the revisions.
  7. Add the page to sidebar.js.
  8. Add, commit, push, and create a new PR.

@adiati98 adiati98 closed this Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📝 documentation 💡 feature A label to note if work is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants