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

In markdown, update summary > heading style #1949

Merged
merged 8 commits into from
Feb 28, 2022
Merged

In markdown, update summary > heading style #1949

merged 8 commits into from
Feb 28, 2022

Conversation

heiskr
Copy link
Contributor

@heiskr heiskr commented Feb 22, 2022

What are you trying to accomplish?

Rendering a heading in a summary tag in Markdown currently looks a bit goofy.

Hello Hello!

Some fantastical text

It would be great if instead it looked more intentional like:

Screen Shot 2022-02-22 at 4 05 28 PM

This would also help the Docs team with accessibility for an upcoming project. For Hubbers, you should see the reference link below the original post of this pull request.

What approach did you choose and why?

I looked and tried to find the best file available for this. There doesn't appear to be any custom styling for details/summary in Markdown, just some general styles. Not sure if this would be a good global property, or better limited to just Markdown rendering.

What should reviewers focus on?

Hmmm... not sure :) Happy to make any needed changes.

Are additional changes needed?

  • No, this PR should be ok to ship as is. 🚢

cc @emilyistoofunky @parkerbxyz

@heiskr heiskr marked this pull request as ready for review February 23, 2022 00:08
@heiskr heiskr requested a review from a team as a code owner February 23, 2022 00:08
@heiskr heiskr requested a review from simurai February 23, 2022 00:08
@changeset-bot
Copy link

changeset-bot bot commented Feb 23, 2022

🦋 Changeset detected

Latest commit: 8c29aea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

This looks like a good improvement. 👍

One thing that feels a bit weird is having that border not span across the whole width (due to being inline-block). I played a bit around but couldn't really find a good solution.

Screen Shot 2022-02-25 at 16 45 29

So maybe for now we could remove that border and also the extra padding? Maybe by adding:

.markdown-body {
  summary {
    h1,
    h2 {
      border-bottom: none;
      padding-bottom: 0;
    }
  }
}

Screen Shot 2022-02-25 at 16 45 06

@heiskr
Copy link
Contributor Author

heiskr commented Feb 25, 2022

It looks like your welcome Actions workflow may need some additional token permissions to run on non-member pull requests:

permissions:
  contents: read
  pull-requests: write

https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

👍

@simurai simurai enabled auto-merge (squash) February 28, 2022 07:26
@simurai simurai merged commit dab8319 into primer:main Feb 28, 2022
@primer-css primer-css mentioned this pull request Feb 28, 2022
@heiskr heiskr deleted the patch-1 branch February 28, 2022 16:24
@parkerbxyz
Copy link

👋 @heiskr @simurai Today I noticed the anchor links for the headers overlap the drop down icon for the lists:

Screen.Recording.2022-04-27.at.4.55.08.PM.mov

I’m posting it here first because I’m assuming it’s related to this PR, but I’d be happy to open a new issue for this if you’d like.

@simurai
Copy link
Contributor

simurai commented Apr 28, 2022

@parkerbxyz Today I noticed the anchor links for the headers overlap the drop down icon for the lists:

Thanks for reporting.. 🙇 this follow-up PR #2048 should fix it.

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