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

Add anchor-workaround for 'small-section-header' and method heading #640

Closed
wants to merge 1 commit into from

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Mar 14, 2020

@jyn514
Copy link
Member

jyn514 commented Mar 16, 2020

r? @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

Just tried this: https://docs.rs/regex/1.3.5/regex/struct.Matches.html#impl-Borrow%3CT%3E

Doesn't look very nice, but it's not from this change. Otherwise, this PR makes things a bit better. So I'm find with merging it. Your call @jyn514. :)

@cynecx
Copy link
Contributor Author

cynecx commented Mar 16, 2020

@GuillaumeGomez Ehrm, it kinda looks "normal" to me. Do you mind taking a screenshot for me?

@GuillaumeGomez
Copy link
Member

With this PR, it gives (on firefox):

Screenshot from 2020-03-16 18-44-34

@jyn514
Copy link
Member

jyn514 commented Mar 16, 2020

That looks like it still has the anchor issue to me. The prod version running right now has the same issue.

@cynecx
Copy link
Contributor Author

cynecx commented Mar 16, 2020

I've just checked that out and it seems like this trick won't work with impl headings because the styling for <h3> uses display: flex. I honestly don't know what we can do here except use a wrapper element so display: flex doesn't affect the ::before-pseudoelement.

@GuillaumeGomez
Copy link
Member

I still think that pushing all the content under the docs.rs top banner is the best solution.

@jyn514
Copy link
Member

jyn514 commented Mar 16, 2020

I still think that pushing all the content under the docs.rs top banner is the best solution.

The problem with this is that overflow: auto seems to cause lots of other issues. I don't know CSS very well but @cormacrelf said in #595 (comment) that overflow: auto causes lots of problems on mobile.

@GuillaumeGomez
Copy link
Member

If this is mobile only, then we can implement mobile-specific fixes.

@cynecx
Copy link
Contributor Author

cynecx commented Mar 17, 2020

Honestly, I feel bad :/. Introducing that PR which allowed better chromium search experience doesn't really justify the broken things right now.

I still think that pushing all the content under the docs.rs top banner is the best solution.

Do you mean not having a sticky header at all?

@jyn514
Copy link
Member

jyn514 commented Mar 17, 2020

Honestly, I feel bad :/

Oh don't worry, the CSS has been broken long before you started making changes 😆. We might end up reverting your change though.

@cynecx
Copy link
Contributor Author

cynecx commented Mar 17, 2020

@jyn514 Yes I kinda agree, I thought this could have worked by applying various fixes here and there but this requires a more “thorough” rework/redesign (probably starting with rustdoc) I guess.

If @GuillaumeGomez agrees too, I would close this PR and revert all the changes related to this and perhaps open a new tracking issue.

@GuillaumeGomez
Copy link
Member

Do you mean not having a sticky header at all?

Yes and no. Depends how we do it. If we make the whole content start below the header, it won't go under it.

@jyn514
Copy link
Member

jyn514 commented Mar 17, 2020

If @GuillaumeGomez agrees too, I would close this PR and revert all the changes related to this and perhaps open a new tracking issue.

Ok, please do this.

@jyn514 jyn514 closed this Mar 17, 2020
cynecx added a commit to cynecx/docs.rs that referenced this pull request Mar 17, 2020
jyn514 pushed a commit that referenced this pull request Mar 17, 2020
@jyn514
Copy link
Member

jyn514 commented Mar 17, 2020

@jyn514 Yes I kinda agree, I thought this could have worked by applying various fixes here and there but this requires a more “thorough” rework/redesign (probably starting with rustdoc) I guess.

Note that we cannot change the generated HTML on past documentation. Doing so would at a minimum take several months of compute time, and might also cause builds to fail that previously passed. See #464.

@GuillaumeGomez
Copy link
Member

No need to change the HTML, just overload the CSS with our own.

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.

3 participants