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] Make the whole header clickable #39603

Merged
merged 12 commits into from
Apr 12, 2024

Conversation

MoazMirza-13
Copy link
Contributor

@MoazMirza-13 MoazMirza-13 commented Oct 25, 2023

Added a feature by which user can click on heading too.
fix #39350

Preview: https://deploy-preview-39603--material-ui.netlify.app/material-ui/react-autocomplete/

Untitled_.Oct.25.2023.1_06.PM.webm

@mui-bot
Copy link

mui-bot commented Oct 25, 2023

Netlify deploy preview

https://deploy-preview-39603--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 2e79c3a

@MoazMirza-13 MoazMirza-13 changed the title [docs-infra] feat: clickable header issue#39350 [docs-infra] feat: clickable header Oct 25, 2023
@MoazMirza-13 MoazMirza-13 changed the title [docs-infra] feat: clickable header [docs-infra] fix clickable header Oct 25, 2023
@MoazMirza-13 MoazMirza-13 changed the title [docs-infra] fix clickable header [docs-infra] Make the whole header clickable Oct 25, 2023
@danilo-leal danilo-leal added the scope: docs-infra Specific to the docs-infra product label Oct 25, 2023
@danilo-leal danilo-leal requested a review from a team October 25, 2023 12:05
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.

Sorry for the delay, My comments were in pending state since few days I don't know why 🫣

Basically tests are not an issue, we can fix them after agreeing on the new DOM structure 👍

packages/markdown/parseMarkdown.test.js Outdated Show resolved Hide resolved
packages/markdown/parseMarkdown.test.js Outdated Show resolved Hide resolved
packages/markdown/parseMarkdown.test.js Outdated Show resolved Hide resolved
packages/markdown/parseMarkdown.js Outdated Show resolved Hide resolved
@MoazMirza-13
Copy link
Contributor Author

Sorry for the delay, My comments were in pending state since few days I don't know why 🫣

Basically tests are not an issue, we can fix them after agreeing on the new DOM structure 👍

@alexfauquette so am i suppose to leave this tests on you guys ?

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.

Could you do the same modification to ClassesSection, CSSSection, PropertiesSection and SlotSection which are the same kind of title but for API page rendering

https://github.com/mui/material-ui/blob/master/docs/src/modules/components/ApiPage/sections/ClassesSection.tsx/#L72-L84

Basically the idea would be that all the "anchor-link" of the codebase get modified the same way

docs/src/modules/components/MarkdownElement.js Outdated Show resolved Hide resolved
docs/src/modules/components/MarkdownElement.js Outdated Show resolved Hide resolved
packages/markdown/parseMarkdown.js Outdated Show resolved Hide resolved
@alexfauquette
Copy link
Member

so am i suppose to leave this tests on you guys ?

Yes, we can fix them when PR is ready to be merged

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Quick design note, though — Ideally, we'd be able to pull this off without any design changes (like adding an underline or removing styles from the anchor button). 😬

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 4, 2023

In terms of UX, I would be in favor of implementing this in a way that doesn't impact the header text selection experience. I feel that selecting a header as a text copy is as much a frequent use case as wanting to copy the header anchor.

@oliviertassinari oliviertassinari added the enhancement This is not a bug, nor a new feature label Nov 4, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 5, 2024
@alexfauquette alexfauquette self-assigned this Apr 5, 2024
@alexfauquette alexfauquette changed the base branch from master to next April 5, 2024 09:04
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 5, 2024
@alexfauquette
Copy link
Member

@danilo-leal I moved forward this PR

I've modified 3 kind of pages:

The design should be unchanged, except that:

  1. titles are now links to the hash
  2. it's harder to copy-past because it's a link 😅

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Looking great! 👍 Sorry it took me a few days to look at your latest iteration.

@alexfauquette alexfauquette merged commit 48191a8 into mui:next Apr 12, 2024
22 checks passed
alexfauquette added a commit to Janpot/material-ui that referenced this pull request Apr 12, 2024
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

(I'm landing here from a bug I noticed on the docs, trying to find the origin).

It's great to have the headers clickable, it saves time. But I think that it's too eager, I think that we should:

  1. Be able to use double click to select the whole header
  2. Be able to select any words of a header
  3. When selecting any words of a header not triggering the header anchor

I feel like I frequently copy and paste content in the headers to search. It's impossible now if the content is in the middle:

Screen.Recording.2024-04-21.at.01.52.15.mov

https://deploy-preview-39603--material-ui.netlify.app/material-ui/react-button/#cursor-not-allowed

Stripe docs is the closest to a great execution that I could find (do 1 and 2 well), the only thing I would fix there is click to select shouldn't click if some text is selected (fail at 3).

In our case, we fail at 1, 2, and 3. Can we fix this? Thanks


Also see the invalid HTML structure: #39603 (comment).

</svg>
<a aria-labelledby={hash} className="title-link-to-anchor" href={`#${hash}`} tabIndex={-1}>
{getTranslatedHeader(t, hash)}
<div className="anchor-icon">
Copy link
Member

@oliviertassinari oliviertassinari Apr 20, 2024

Choose a reason for hiding this comment

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

We can't do this: https://validator.w3.org/nu/?doc=https%3A%2F%2Fnext--material-ui.netlify.app%2Fmaterial-ui%2Fgetting-started%2F

SCR-20240421-cero

I guess this would fix the regression:

Suggested change
<div className="anchor-icon">
<span className="anchor-icon">

Fixing this in #42085.

Comment on lines -776 to +783
'& a, & a code': {
'& a:not(.title-link-to-anchor), & a:not(.title-link-to-anchor) code': {
Copy link
Member

Choose a reason for hiding this comment

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

I'm reverting this approach, it increased the specificity, leading to overrides other styles: #42735. It seems that we don't need this more complex CSS selector, we can solve the same problem with the CSS injection order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design This is about UI or UX design, please involve a designer enhancement This is not a bug, nor a new feature scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs-infra] Make the whole header clickable
5 participants