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

Update about us with all-contributors info #1919

Merged
merged 11 commits into from
Jun 16, 2022
Merged

Conversation

tlylt
Copy link
Contributor

@tlylt tlylt commented May 3, 2022

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Overview of changes:
closes #1621

  • Adjust all-contributorsrc to make automatic changes to both the README.md and the docs/about.md
  • Re-organize the about-us page to show the contributors
  • Put past records below under "past members"
  • add mentoring and pojectManagement emoji for prof and zeyu, as an indication of project mentor and lead, as well as adding some other emojis

Anything you'd like to highlight / discuss:

  • whether to keep the past records or remove entirely
  • whether this is enough or should we present the core teams separately
  • whether we should use more emojis to cover concepts of project-lead, senior-dev, core team
    • and if this is the case, do we need to keep it up to date by removing the emoji when necessary...will thus require updates to the dev docs

Testing instructions:
https://deploy-preview-1919--markbind-master.netlify.app/about.html

Proposed commit message: (wrap lines at 72 characters)
Adjust about-us page to read all-contributors info


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

docs/about.md Show resolved Hide resolved
Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Thanks @tlylt

  • whether this is enough or should we present the core teams separately
  • whether we should use more emojis to cover concepts of project-lead, senior-dev, core team

I have no preference on this. I am fine with both. @MarkBind/active-devs Any thoughts on this?

and if this is the case, do we need to keep it up to date by removing the emoji when necessary...will thus require updates to the dev docs

In both cases, I think we will still have to update the dev docs when the core team changes (new members joining etc).

docs/about.md Outdated
<!-- ALL-CONTRIBUTORS-LIST:END -->
</div>

<panel header="Past Record" type="seamless">
Copy link
Contributor

Choose a reason for hiding this comment

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

The panel doesn't seem to be working on the preview site?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jonahtanjz, thanks for the heads-up. Interestingly, this is an unexpected hydration issue, in which the all-contributors-cli generates a table without <tbody> element, which syntactically is fine but would error out for Vue due to the mismatch in HTML content. I have followed up the issue in the source repo here, will wait and see if there are any updates.

In the meantime, it is possible to manually add <tbody> into the HTML generated by all-contributors, but it seems like it will be erased if any further update is made automatically, which defeats the purpose of using the bot.

So at the moment, I hack a solution by moving the past record above the contributor table so that the hydration issue will not affect it. See https://deploy-preview-1919--markbind-master.netlify.app/about.html

Copy link
Contributor

Choose a reason for hiding this comment

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

  • whether this is enough or should we present the core teams separately
  • whether we should use more emojis to cover concepts of project-lead, senior-dev, core team
  1. I think the current design works (and is less effort to maintain), unless there's some automation.
  2. I think adding more emojis would be nice, separating the roles can help new users identify who to contact if they need help.

Copy link
Contributor

Choose a reason for hiding this comment

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

whether this is enough or should we present the core teams separately

Agree its enough as well.

whether we should use more emojis to cover concepts of project-lead, senior-dev, core team

On the same lines, I think this is fine too (unless it can be automated). Also as I can't find suitable emojis here to represent these roles: https://allcontributors.org/docs/en/emoji-key. 👀

We can preserve the current formatting (just text, take it out of the panel) for these roles (which should be limited in size) to make maintaining it easier.

whether to keep the past records or remove entirely

My 2 cents:

  • extract and preserve the pm / senior dev list out of the panel per the above problem. (no need for the all-contributors icon). something simple like this https://chromium.googlesource.com/external/github.com/grpc/grpc-go/+/refs/heads/master/MAINTAINERS.md.

    maybe these sections (expand / remove the panel):

    • Current team - pms + senior devs
    • Emeritus - though not sure if we want to move senior / past devs to this list as well as it would be already quite large retroactively
      • ps: we also don't currently have "senior dev" https://markbind.org/about.html, think we could check with @damithc especially on the intention for this. Including the current ones at least would definitely be nice imo.
  • use all-contributors for all the other ones to reduce the maintainence work + duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • whether we should use more emojis to cover concepts of project-lead, senior-dev, core team

No preference, same as Ze Yu - can't find a suitable emoji from all-contributors to represent these

  • whether this is enough or should we present the core teams separately

I'm concerned about the maintainability of the "current team" - do we define it as all current CS3281/2 students and mentors, or all people currently actively contributing to the project? It seems that we'll have to manually maintain this section specifically, which might get forgotten if we happen to have independent contributors not part of CS3281/2.

  • whether to keep the past records or remove entirely

I think we should keep past project leads/mentors (and maybe maintainers) as it could be interesting information for new devs who wish to know more about MarkBind's project history. I feel there's no harm removing past contributors from the panel since they would already be listed under the "Contributors" section (from all-contributors) below.

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 concerned about the maintainability of the "current team" - do we define it as all current CS3281/2 students and mentors, or all people currently actively contributing to the project? It seems that we'll have to manually maintain this section specifically, which might get forgotten if we happen to have independent contributors not part of CS3281/2.

Should be everyone who is actively engaged in the project.

I think we should keep past project leads/mentors (and maybe maintainers) as it could be interesting information for new devs who wish to know more about MarkBind's project history. I feel there's no harm removing past contributors from the panel since they would already be listed under the "Contributors" section (from all-contributors) below.

Yes, it's better to keep if we can.

@tlylt
Copy link
Contributor Author

tlylt commented Jun 3, 2022

Hello guys, thanks for the feedback so far, I have made the following changes:

  • Add some emojis for contributors
  • Keep the past record at the end of the page
  • Move the mentor + dev team section up before the contributors
  • Add a temporary <br><br> fix to deal with the fact that all-contributors does not generate compliant HTML which breaks the hydration. (Not ideal, but the all-contributors-cli project seems to be stagnant at the moment, I intend to put up a PR and follow up on that and see how it goes. In the mean time, this temporary fix will allow us to use the all-contributor table in this page without obvious issues)

My proposed plan for maintaining this page is as follows:

  • Whenever there's a contributor, the usual all-contributor bot will help update this page along with the README, no further action is required
  • Whenever there's a senior role update, which perhaps happens once every semester or two, manually update the dev team section.

Review/feedback is welcomed:)
https://deploy-preview-1919--markbind-master.netlify.app/about.html

@tlylt tlylt requested a review from ong6 June 3, 2022 16:27

:glyphicon-send: You can **email us** at `markbind` at `comp.nus.edu.sg`
**Dev Team**:
Copy link
Contributor

Choose a reason for hiding this comment

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

^ Missing the most recent batch (you guys) + some date touchups

* [**Ang Ze Yu**](https://www.github.com/ang-zeyu): **_Project Lead_** since Jan 2021, Member since Jan 2020
* [**Jonah Tan Jun Zi**](https://www.github.com/jonahtanjz): _Senior Dev_ since Apr 2021, _Member_ since Jan 2021
* [**Koh Rayson**](https://www.github.com/raysonkoh): _Senior Dev_ since Apr 2021, _Member_ since Jan 2021
* [**Ong Wei Xiang**](https://www.github.com/wxwxwxwx9): _Senior Dev_ since Apr 2021, _Member_ since Aug 2020
* [**Ryo Chandra Putra Aramanda**](https://www.github.com/ryoarmanda): _Senior Dev_ since Apr 2021, _Member_ since Aug 2020
* [**Hannah Chia Kai Xin**](https://www.github.com/kaixin-hc): _Senior Dev_ since Apr 2022, _Member_ since Jan 2022
* [**Jovyn Tan Li Shyan**](https://www.github.com/jovyntls): _Senior Dev_ since Apr 2022, _Member_ since Jan 2022
* [**Liu Yong Liang**](https://www.github.com/tlylt): _Senior Dev_ since Apr 2022, _Member_ since Aug 2021
* [**Ong Jun Xiong**](https://www.github.com/ong6): _Senior Dev_ since Apr 2022, _Member_ since Aug 2021

My proposed plan for maintaining this page is as follows:

Sounds good 👍

Hi @jonahtanjz, thanks for the heads-up. Interestingly, this is an unexpected hydration issue, in which the all-contributors-cli generates a table without <tbody> element, which syntactically is fine but would error out for Vue due to the mismatch in HTML content. I have followed up the issue in the source repo here, will wait and see if there are any updates.

The hydration issue seems to be causing the header runtime detection to not-run as well (and the first heading to be "squeezed" against the header). Maybe we can add a mt-3 margin to the first heading.

Looks really nice otherwise, thanks for consolidating this! @tlylt

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Pushed some most recent updates confirmed with @damithc

@tlylt you can merge this in if it looks ok :)

@tlylt tlylt merged commit f478cdd into MarkBind:master Jun 16, 2022
@tlylt tlylt deleted the update-about-us branch June 16, 2022 11:51
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.

6 participants