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

Adds contributors to docs #6900

Merged
merged 13 commits into from
Apr 30, 2024
Merged

Adds contributors to docs #6900

merged 13 commits into from
Apr 30, 2024

Conversation

alvinometric
Copy link
Contributor

@alvinometric alvinometric commented Apr 22, 2024

Hello!

We wanted to make the docs less impersonal, so we decided to add contributors. Now each doc page that has an editUrl (i.e, isn't generated) shows a list of everyone that contributed to it.

This list is generated by:

  1. Running swizzle on the DocItem/Footer in Docusaurus.
  2. Grabbing metadata for the current file using an internal docusaurus API (Thank you to @homotechsual for the help there)
  3. Getting the commits to the file in question with the GitHub API

image

Here's the command I ran, for posterity npm run swizzle @docusaurus/theme-classic DocItem/Footer -- --wrap

Discussion points

  1. Design. What do you think of the layout?
  2. Right now I'm hardcoding the info of Unleash team members. This creates a small maintenance burden, but it's something we wanted to add.

Copy link

vercel bot commented Apr 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 30, 2024 1:31pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Apr 30, 2024 1:31pm

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

This is cool 😄 Left a couple comments on the implementation in the code, but overall I think it's nice 👏🏼

Now, on your discussion points:

  1. I think the design is fine. It feels a bit wonky in that if you have a job title, then that pushes your username up etc., but it's not a big deal. If you want some design input, how about talking to Fredrik?

  2. Hard coding the Unleash members. I .. don't love it, but I think it's fine if we think that it's good for visibility. I guess we could fetch that data from github somehow too, but not sure it's worth the complexity. I'm not convinced we should do titles, though. How about just showing a small Unleash logo next to Unleash members or something? Not a show stopper, but just an idea. However, if you do go ahead: those titles aren't correct, and I don't think we should use titles that aren't.

More importantly: the SDK overview doesn't work anymore: https://unleash-docs-git-alvin-docs-contributors-unleash-team.vercel.app/reference/sdks, so I think there's some cases you're not catching in the code 👀

}, []);

if (!contributors.length) {
return <h3>Fetching contributors...</h3>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of change the header, could we have the header "Contributors" regardless, and instead using this as content? Feels like this should have a p tag instead of a header tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is then I have to nest everything else in an if. I'm not sure it justifies the UX benefits

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh? Not sure I understand what you mean here. You could just do this instead, right?

        <div className={styles.contributors}>
            <h3>Contributors</h3>
            <p>Fetching contributors ...</p>
        </div>

That feels more semantically correct to me. Changing headers dynamically seems a bit strange to me due to landmark navigation.

website/src/theme/DocItem/Footer/contributors.jsx Outdated Show resolved Hide resolved
website/src/theme/DocItem/Footer/contributors.jsx Outdated Show resolved Hide resolved
website/src/theme/DocItem/Footer/contributors.jsx Outdated Show resolved Hide resolved
website/src/theme/DocItem/Footer/contributors.jsx Outdated Show resolved Hide resolved
website/src/theme/DocItem/Footer/contributors.jsx Outdated Show resolved Hide resolved
@thomasheartman
Copy link
Contributor

It seems you fixed the crashing page error. Did you find out what was wrong there? For the future, we might want to consider adding feature flags to the docs again, so that we can disable new features like this if they slip by. It was only by accident that I caught the crash 🤔 But that's for later, for sure.

Regarding the design, it looks kinda messy when you have lots of contributors:
image

Should we consider formatting it all vertically instead? Or how about just showing the images? That way we know that all images will be the same size, so it looks neat 💁🏼 Or we give each one a set size, and cut text if it overflows?

@thomasheartman
Copy link
Contributor

I suspect that using just the avatars will be enough. It's also consistent with how we (and many other people) do it on github, for instance

@thomasheartman
Copy link
Contributor

Oh, and we should also find a way to deal with SDK pages (such as https://unleash-docs-git-alvin-docs-contributors-unleash-team.vercel.app/reference/sdks/flutter). They currently only show the header and never find the right data:

image

This is because these docs aren't hosted in the Unleash repo itself, but on the various SDKs/Edge/Proxy repos. These docs are essentially just the readme for those repos. However, the edit page link still works and takes you to the right repo.

You can find more information about this in the remote-content directory. The docusaurus config contains the following imports:

const { sdks } = require('./remote-content/sdks');
const { docs: edgeAndProxy } = require('./remote-content/edge-proxy');

Those files contain all the SDK links, where they get sourced from, etc.

@alvinometric
Copy link
Contributor Author

Hah, you reviewed before I could comment, so to address the initial points:

  1. Yes, @FredrikOseberg any inputs on the design?
  2. For sure, I'll rectify the titles once we're good on the code side.

Re: the SDKs, I should be able to retrieve the contributors for these too, I just need to tweak the code a bit.

@FredrikOseberg
Copy link
Contributor

Hah, you reviewed before I could comment, so to address the initial points:

  1. Yes, @FredrikOseberg any inputs on the design?
  2. For sure, I'll rectify the titles once we're good on the code side.

Re: the SDKs, I should be able to retrieve the contributors for these too, I just need to tweak the code a bit.

I think the easiest path forward is to only display avatars and to display more information on hover/focus. The key to keep it looking good and consistent is to make sure everything has the same size, so when we include user input this quickly gets hard to manage because titles and names will differ in size leading to the perceived disorganized layout. This is similar to how for example github does it:

Skjermbilde 2024-04-23 kl  13 56 19

@alvinometric
Copy link
Contributor Author

alvinometric commented Apr 23, 2024

What about adding a logo for Unleash team members? Kinda like a verified tag?

@thomasheartman
Copy link
Contributor

I like that, yeah. Had the same idea 🙋🏼

@FredrikOseberg
Copy link
Contributor

Sounds good! I like it.

@alvinometric
Copy link
Contributor Author

24.April.2024.-.Brave-Browser---SDK-overview--Unleash---24-April-2024.mp4

Here's what it looks like now with icons and more info on hover

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Nice, yeah, that looks pretty cool!

A few things we could fix (though not urgent, so happy to have that done in a follow-up:

  1. The keyboard focus. The outline looks misaligned (I'm guessing that's because you set border radius on the image rather than the link) and there's no effect when you focus it (though whether we want it to pop up on keyboard focus or not is a decision to make).
    image

  2. It's subtle, but the focus outline doesn't match the shape of the avatar when it zooms in. Notice the white edge on the lower right area of this avatar:
    image

website/src/theme/DocItem/Footer/contributors.module.scss Outdated Show resolved Hide resolved
website/src/theme/DocItem/Footer/contributors.module.scss Outdated Show resolved Hide resolved
website/static/img/logo.svg Outdated Show resolved Hide resolved
@alvinometric
Copy link
Contributor Author

I think I've addressed everything, but to come back:

  1. I think the focus is fixed now that I've removed the margins in favour of the gap
  2. I can't reproduce it, again it's possible that it's a result of relying on gap

Copy link
Contributor

@nnennandukwe nnennandukwe left a comment

Choose a reason for hiding this comment

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

👍 lgtm

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Nice, yeah! I'm happy for this to go in now, but I think there's a few things we should update (but please, make it another PR, so we can get this in):

  1. I'm still not convinced we should have titles for Unleash people, especially not when they're not correct titles. I'd just go with "Unleash employee" or something (maybe grab a special one for Ivar who can never not be the founder 💁🏼 )
  2. The keyboard focus styles are still weird. I'd be happy to look into this after you've merged if you want? It also acts differently for Unleash people than it does for outside contributors:
    image
    image

Dynamically updating a header still feels iffy to me. Feels like it'll be bad for accessibility in terms of navigation etc, but I can't point to anything specific, so I might just be overly worried. I also think it'd make more sense to update the content than the header, but .. eh, it's not the end of the world.

Comment on lines 33 to 35
console.log(commits);
const contributors = getContributors(commits);
console.log(contributors);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, we probably wanna remove these logs, though:

Suggested change
console.log(commits);
const contributors = getContributors(commits);
console.log(contributors);
const contributors = getContributors(commits);

@alvinometric
Copy link
Contributor Author

Thanks again, so

  1. Yeah, let's make this another PR
  2. Thanks for keeping me honest with this, I've updated it with the right titles now
  3. I'm just retuning null now, better than having an empty header

@alvinometric alvinometric merged commit 4ad56e8 into main Apr 30, 2024
8 checks passed
@alvinometric alvinometric deleted the alvin/docs-contributors branch April 30, 2024 14:19
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.

4 participants