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

docs: Add contributing guidelines #8222

Merged
merged 11 commits into from Feb 13, 2023

Conversation

Bad3r
Copy link
Collaborator

@Bad3r Bad3r commented Jan 5, 2023

Adding a contributing guidelines to Logseq.
These guidelines are based on and inspired by the Angular and VS Code projects contributing guidelines.

Questions for Logseq team:

  • Do we have a Corporate CLA document that is different from the individual CLA document?

This PR should remain open until these questions are answered, and the document is updated. Preferably, all Logseq team members should review this document.

In the future, we might want to consider making the document shorter and utilize GitHub wiki for all dev related live documents

@Bad3r
Copy link
Collaborator Author

Bad3r commented Jan 10, 2023

@logseq-cldwalker when you get a chance, I would appreciate any feedback on this PR.

@logseq-cldwalker
Copy link
Collaborator

@Bad3r I can take a look tomorrow. Fyi the core team uses https://github.com/logseq/logseq/pulls?q=is%3Apr+is%3Aopen+-label%3A%3Atype%2Fhold+draft%3Afalse+NOT+WIP+in%3Atitle for looking for PRs that are ready for review. I assumed this wasn't ready b/c of the "WIP"

@Bad3r Bad3r changed the title [WIP] docs: Add contributing guidelines docs: Add contributing guidelines Jan 12, 2023
@Bad3r Bad3r added the :type/dev This label is used to indicate that an issue or PR is related to development tasks or changes that label Jan 12, 2023
@Bad3r
Copy link
Collaborator Author

Bad3r commented Jan 12, 2023

Oh good to know!
Its not ready for merge but it is ready for review and feedback

@logseq-cldwalker logseq-cldwalker removed the request for review from sawhney17 January 26, 2023 20:39
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@Bad3r Thanks for nudging us to get a first version of this out. I finally got around to reviewing this as most of the team will be back soon. I've left some small feedback and left one open question for the team

CONTRIBUTING.md Outdated
```


## <a name="rules"></a> Coding Rules
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what we are striving towards but I don't think we consistently enforce this level of testing or documentation. I can just rewrite this section after this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@logseq-cldwalker I guess the question is; should this describe how it's done now or use this as an opportunity to write what we want to transition to in the near future.

Could you give me a bit more insight into what you have in mind for this section? . Also, would you mind making the changes before merging this PR? It would be great to make sure we're on the same page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd take longer to explain than to just do it. I'll rewrite this at the end of this PR as this something for engineers to fill in

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
- **Note:** Because the developers need to copy and paste the code snippet, including a code snippet as a media file (i.e. .gif) is not sufficient.
- Errors from the Dev Tools Console (open from the menu: View > Toggle Developer Tools or press CTRL + Shift + i)

You can file new issues by selecting from our [new issue templates][new-issue] and filling out the issue template.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: It could be nice to just point to the template link and then use this section to elaborate on things that aren't in the templates like the Dev Tools Console or tools in the new bug report page

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@logseq-cldwalker don't you think that's a better fit for the docs folder?
I do think such section will add value; We can also link to it from the issue template.
The only reason I am hesitant to include it in this document is that it will be long and will require screenshots etc..

CONTRIBUTING.md Outdated Show resolved Hide resolved
@logseq-cldwalker
Copy link
Collaborator

logseq-cldwalker commented Jan 26, 2023

Do we have a Corporate CLA document that is different from the individual CLA document?

Don't think we have a corporate CLA but if we plan on doing it later @andelf would know

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated

## <a name="question"></a> Got a Question or a Problem?

Do not open issues for general support questions or feature requests as we want to keep GitHub issues for bug reports.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure our process, but I feel like something should be said about GitHub discussions here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bendyorke honestly I am trying to get GitHub Discussions shutdown. I do not think its a good idea to have it. We should consolidate it to the Logseq forum.

@logseq-cldwalker curious if you have any thoughts on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Bad3r I haven't done much with github discussions. Agree that it'd be nice to not support it if we don't have to. Will defer to @cnrpman since he's worked with it more. I think it's ok to omit github discussions for now until we now what we are doing with it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cnrpman any thoughts on closing GitHub discussions and consolidating it into the Logseq forum?

CONTRIBUTING.md Outdated Show resolved Hide resolved
@logseq-cldwalker
Copy link
Collaborator

@Bad3r Feel free to address comments soon. I've asked the team internally to look at this so if we don't hear from anyone else, would be great to finish the PR this week

@logseq-cldwalker
Copy link
Collaborator

@Bad3r Would be great to address feedback as most of the feedback is minor and would like to clean up the review queue. I can finish it up if it's still hanging out

@Bad3r
Copy link
Collaborator Author

Bad3r commented Feb 4, 2023

@Bad3r Would be great to address feedback as most of the feedback is minor and would like to clean up the review queue. I can finish it up if it's still hanging out

Sounds good, will get it done in the next 24 hours.

@Bad3r
Copy link
Collaborator Author

Bad3r commented Feb 5, 2023

Addressed the feedback thus far and added instructions on naming PRs instead of commits.

- Images, animations, or a link to a video showing the issue occurring
- A code snippet that demonstrates the issue or a link to a code repository the developers can easily pull down to recreate the issue locally
- **Note:** Because the developers need to copy and paste the code snippet, including a code snippet as a media file (i.e. .gif) is not sufficient.
- Errors from the Dev Tools Console (open from the menu: View > Toggle Developer Tools or press CTRL + Shift + i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: This link may be helpful, as @cnrpman usually uses it to guide people that have issues.

https://help.figma.com/hc/en-us/articles/360041949073-Use-Chrome-Developer-Tools

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good guide. This is what @logseq-cldwalker was asking about.

Optional: It could be nice to just point to the template link and then use this section to elaborate on things that aren't in the templates like the Dev Tools Console or tools in the new bug report page

I do not link to external 3rd parties. I prefer that links are either from Logseq.com or GitHub.com.

It's best we can create our own document with instructions on using Dev tools and other tools that can be helpful in diagnosing issues.

For example; we can add a section on diagnosing performance issues, etc..

We can use this guide as the base for ours.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@Bad3r
Copy link
Collaborator Author

Bad3r commented Feb 8, 2023

@logseq-cldwalker any notes on the changes?

@logseq-cldwalker
Copy link
Collaborator

Sorry I didn't get to this. Aim to get to this on Monday

Also tweaked some wording and added emojis of course
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@Bad3r Thanks for kicking off a great contributing doc! 👍 🎉 I've added some emojis, rewritten the PR section and added a how to help section

CONTRIBUTING.md Outdated

You can file new issues by selecting from our [new issue templates][new-issue] and filling out the issue template.

### <a name="submit-pr"></a> Pull Requests (PR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In re-reading this section, we don't follow most of these PR conventions. I've rewritten this section to have the focus be on the PR submission and the conventions that are actively used

@logseq-cldwalker logseq-cldwalker merged commit e5f5e9b into logseq:master Feb 13, 2023
logseq-cldwalker added a commit that referenced this pull request Feb 14, 2023
@Bad3r Bad3r deleted the docs/add-CONTRIBUTING.md branch February 16, 2023 19:32
sallto pushed a commit to sallto/logseq that referenced this pull request Feb 25, 2023
* docs: Add contributing guidelines

* Add a how to help section and redo PR section

Also tweaked some wording and added emojis of course

* Fix up sub lists

---------

Co-authored-by: situ2001 <yongcong2001@outlook.com>
Co-authored-by: Gabriel Horner <gabriel@logseq.com>
sallto pushed a commit to sallto/logseq that referenced this pull request Feb 25, 2023
sallto pushed a commit to sallto/logseq that referenced this pull request Feb 25, 2023
* docs: Add contributing guidelines

* Add a how to help section and redo PR section

Also tweaked some wording and added emojis of course

* Fix up sub lists

---------

Co-authored-by: situ2001 <yongcong2001@outlook.com>
Co-authored-by: Gabriel Horner <gabriel@logseq.com>
sallto pushed a commit to sallto/logseq that referenced this pull request Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:type/dev This label is used to indicate that an issue or PR is related to development tasks or changes that
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants