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

Make KDS contributor friendly #447

Merged
merged 7 commits into from
Sep 21, 2023

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Aug 29, 2023

Description

This is a sibling to Kolibri's PR learningequality/kolibri#11096 and part of larger efforts to make our issues better discoverable, documentation more friendly, and in the case of KDS also easier development process of components for contributors and any newcomers to this repository.

  • Adds CONTRIBUTING.md and updates README.md to contain only necessary information and a copy of contributing guidelines for better discoverability. Both files now have the same structure as Kolibri's CONTRIBUTING and README (Studio to come).
  • Moves most of the README's content into dev_docs folder and organizes it in to more files while focusing on
    • friendliness and clarity
    • information organized by focusing on necessary steps at first and providing links to more detailed guidelines later
  • Adds a few new guidelines important for starting with KDS development as how to preview updates to components or register a new component
  • Adds a playground page and guidelines on how to use it as the most simple way to preview components updates. Its purpose is to allow new developers start working with KDS easily and just within a few necessary steps without the need to go into detail about building documentation pages or connecting with other projects (that may still be relevant and guidelines are available too, but I think it's unnecessary for very first steps)
  • Add contact links to the issue template chooser (same as in Kolibri)

Steps to test

I think it'd be best to imagine you don't know anything about KDS and read through

Also please try out the new playground page.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change has been added to the changelog

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?

MisRob added 2 commits August 29, 2023 13:31
- Make it friendly for newcomers
- Organize existing guidelines into more files
- Organize information by focusing on important steps at first,
  and providing links to more detailed pages when relevant
- Add a few new guidelines
- Add guidance for contributors to README.md and CONTRIBUTING.md
@MisRob MisRob force-pushed the contributors-docs branch from beb0732 to 6667321 Compare August 29, 2023 11:31
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Thanks so much @MisRob. I have not built the pages in my read through to do a manual QA of the docs build, but overall I think this looks great. I've done my best to put myself in the mindset of someone newer to KDS, but I would love input from some of our team members who use KDS a bit less to see if they have any suggestions.

Overall though, I think the clearer sections about contributing, how to get started, the links to the discussion, etc. are a great improvement! Unless someone has a really specific suggestions, I feel good about moving forward with this and then continuing to refine as we get feedback from community contributors.

@MisRob
Copy link
Member Author

MisRob commented Aug 31, 2023

Thank you, @marcellamaki.

I would love input from some of our team members who use KDS a bit less

Ah yes, that's a good idea. I am thinking of @ozer550 :) Would you find time to read through the documentation and tried the playground? Doesn't need to be this week.

@ozer550
Copy link
Member

ozer550 commented Aug 31, 2023

Thank you, @marcellamaki.

I would love input from some of our team members who use KDS a bit less

Ah yes, that's a good idea. I am thinking of @ozer550 :) Would you find time to read through the documentation and tried the playground? Doesn't need to be this week.

Sure!

@MisRob
Copy link
Member Author

MisRob commented Aug 31, 2023

@ozer550 Wonderful, thanks

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Hi @MisRob! This really looks good, and certainly improves the developer experience as a whole. Like Marcella, I will also leave the approval of this PR to folks that have interfaced with KDS the least 🙂

@akolson
Copy link
Member

akolson commented Sep 1, 2023

Hi @marcellamaki just noting that you click on the Details link in the highlighted area to view a live preview of the changes made without the need to build the pages yourself. 🙂

Screenshot 2023-09-01 at 16 39 48

@marcellamaki
Copy link
Member

Hi @marcellamaki just noting that you click on the Details link in the highlighted area to view a live preview of the changes made without the need to build the pages yourself. 🙂

Screenshot 2023-09-01 at 16 39 48

Wow Today I Learned!! Thank you, Samson 🎉

@ozer550
Copy link
Member

ozer550 commented Sep 5, 2023

I tried the walkthrough and it seems good to me! Just some errors encountered that might be worth mentioning:

 ERROR  ENOSPC: System limit for number of file watchers reached, watch '/home/ozer/workspace/LearningEquality/kolibri-design-system/docs/pages/buttons.vue'

  at FSWatcher.start (internal/fs/watchers.js:165:26)
  at Object.watch (fs.js:1258:11)
  at createFsWatchInstance (node_modules/@nuxt/builder/node_modules/chokidar/lib/nodefs-handler.js:119:15)
  at setFsWatchListener (node_modules/@nuxt/builder/node_modules/chokidar/lib/nodefs-handler.js:166:15)
  at NodeFsHandler._watchWithNodeFs (node_modules/@nuxt/builder/node_modules/chokidar/lib/nodefs-handler.js:331:14)
  at NodeFsHandler._handleFile (node_modules/@nuxt/builder/node_modules/chokidar/lib/nodefs-handler.js:395:23)
  at NodeFsHandler._addToNodeFs (node_modules/@nuxt/builder/node_modules/chokidar/lib/nodefs-handler.js:637:21)
WARN  Error from chokidar (/home/ozer/workspace/LearningEquality/kolibri-design-system/node_modules/vue/dist): Error: ENOSPC: System limit for number of file watchers reached, watch '/home/ozer/workspace/LearningEquality/kolibri-design-system/node_modules/vue/dist/vue.esm.browser.min.js'

Also I think if we could mention the major version for yarn it could be helpful.

The above error got resolved following this stack overflow post guided by @MisRob. Overall it looks good to me!

@MisRob
Copy link
Member Author

MisRob commented Sep 19, 2023

Thank you, @ozer550. I will add those improvements before we merge.

Any blockers here, anyone?

@marcellamaki
Copy link
Member

No blockers from my perspective!

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

I didn't read every line but overall the changes are fantastic and I really appreciate the dev_docs breakdown of everything for contributors. Thanks so much for all of your effort on this!

@MisRob
Copy link
Member Author

MisRob commented Sep 21, 2023

All the feedback is addressed and from messages here and on Slack, it looks it's okay to merge. I'll bypass the need for approval as I'd like to reference some of the new sections in the docs easily for one of the contributors soon.

@MisRob MisRob merged commit 5bd95bb into learningequality:develop Sep 21, 2023
6 checks passed
@MisRob MisRob deleted the contributors-docs branch October 14, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants