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-infra] Add GitHub source link to components #43228

Merged
merged 14 commits into from
Aug 20, 2024

Conversation

Jay-Karia
Copy link
Contributor

@Jay-Karia Jay-Karia commented Aug 8, 2024

Adds a "Source" chip in component header links.
Chip links to their respective folder in packages/mui-material.

sourceChip

Note:

  • The docs page for Progress components has LinearProgress and CircularProgress, the source chip can only link to one page, I have linked to LinearProgress
  • Transfer List page does not have any Source chip, as I could not find any package for it.
  • Base UI and MUI X components are not included.

Part of #36824

@zannager zannager added the docs Improvements or additions to the documentation label Aug 8, 2024
@zannager zannager requested a review from brijeshb42 August 8, 2024 13:53
@aarongarciah aarongarciah requested review from a team and removed request for brijeshb42 August 12, 2024 10:11
@aarongarciah aarongarciah changed the title feat(docs): add github source link to components [docs-infra] Add GitHub source link to components Aug 12, 2024
@aarongarciah aarongarciah added scope: docs-infra Specific to the docs-infra product docs Improvements or additions to the documentation and removed docs Improvements or additions to the documentation labels Aug 12, 2024
@Jay-Karia
Copy link
Contributor Author

@aarongarciah Should we wait for failing CIs ?

@alexfauquette
Copy link
Member

Thanks for the contribution. Effectively, it's a quick win.

I removed the autocolosing because the issue is about API pages. In that case the link should ba done by a script. Otherwise, it would be hard to keep something up to date.

I'm modifying the way to define the path. The URL of the repo and the current branch are left to the config. The markdown only have the take care of the file path. Such that when releasing the new major, this feature does not require extra modifications

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

I left this PR open if someone want to give a second look at my modification. Otherwise I will mere it during at the end of the week

@alexfauquette alexfauquette requested a review from a team August 19, 2024 07:19
Copy link
Member

@bharatkashyap bharatkashyap left a comment

Choose a reason for hiding this comment

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

Looks good to me, the only suggestion is if we want to relook at the order of the chips, and move the GitHub source one to be the first or second to indicate its priority.

@Jay-Karia
Copy link
Contributor Author

Should we close #36824 after merge ?

@alexfauquette
Copy link
Member

alexfauquette commented Aug 20, 2024

Should we close #36824 after merge ?

Answered in #43228 (comment)

TLTR; no ;)

I removed the autocolosing because the issue is about API pages. In that case the link should be done by a script. Otherwise, it would be hard to keep something up to date.

@alexfauquette alexfauquette merged commit e52c45e into mui:next Aug 20, 2024
23 checks passed
@Jay-Karia Jay-Karia deleted the feat/source-links branch August 20, 2024 14:44
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 20, 2024

Are we sure about this change? It feels a bit strange in two ways:

  1. The links to the source are hardcoded. Isn't there room to automatically generate them, as we do for the link to the API?
  2. I'm a bit confused about the UX, a source to a component from the component demo page doesn't make a lot of sense to me as a user. Sometimes it works (simple one-component demo page), some other times it doesn't. I would expect to see this link from each API page.

size="small"
variant="outlined"
rel="nofollow"
href={`${process.env.SOURCE_CODE_REPO}/tree/${process.env.SOURCE_GITHUB_BRANCH}/${headers.githubSource}`}
Copy link
Member

@oliviertassinari oliviertassinari Aug 20, 2024

Choose a reason for hiding this comment

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

This link we break once we version the docs pages: Why not do the same thing as with the link to the source for the demos?

githubLocation={`${process.env.SOURCE_CODE_REPO}/blob/v${process.env.LIB_VERSION}${fileNameWithLocation}`}

Copy link
Member

Choose a reason for hiding this comment

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

Was not aware of this part of the code

Copy link
Member

Choose a reason for hiding this comment

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

Alright, then I assume we can move to do the same 😄

@alexfauquette
Copy link
Member

alexfauquette commented Aug 21, 2024

Are we sure about this change?

  1. About generating the links. I don't think since some pages have multiple components. It would require to define a way to pick one of them
  2. About the UX for me those chips are entry points to additional documentation (the WARAI spec, the material design spec) In that context the source is an entry point to where components are. I see that more like a junior friendly introduction to the source code.

About having links in API pages, I agree it would be more reliable and could be automated.

For me they solve different issues:

  • I'm on the API page because. I'm looking for something specific. Then I need to read the code of the actual component
  • I'm on the Demo page. I'm looking for something general. Just knowing where components are defined is good enough for my purpose

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 21, 2024

  1. About generating the links. I don't think since some pages have multiple components. It would require to define a way to pick one of them

@alexfauquette Maybe we could have a default logic that picks the first component listed in the components: field, and still allow people to manually specify a custom one?

  1. I see that more like a junior friendly introduction to the source code.

Would it be better if we were to only show the action if there is a single entry in the components: field? I don't know, when I land on https://deploy-preview-43228--material-ui.netlify.app/material-ui/react-accordion/ and click on Source, I was expecting to see the source of the whole component. Only seeing the root component confuses me.

@colmtuite I'm curious about your view on this. https://www.radix-ui.com/primitives/docs/components/switch has a link to the source, which makes sense in this context and in the context of Base UI because of how the sources are organized: https://github.com/mui/base-ui/tree/master/packages/mui-base/src/Menu. We basically say to the developers, that if you import a Menu, you won't get tree shaking in dev mode for the menu components that you don't use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants