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 footnote styles #1616

Merged
merged 10 commits into from
Sep 29, 2021
Merged

Add footnote styles #1616

merged 10 commits into from
Sep 29, 2021

Conversation

talum
Copy link
Contributor

@talum talum commented Sep 22, 2021

Adding footnote styles to Primer.

While enabling footnote support, we iterated on the styles within dotcom. Now that they seem fairly stable, it'd be great to finish them up and move them.

Here's an example:

Screen Shot 2021-09-27 at 9 14 03 AM

and a gif showing the :target styles:
footnotes_readme


Q: Since these new updates aren't in dotcom yet, I'm wondering, what's the release time on a change like this? 🤔 (In other words, is it worthwhile for me to make the corresponding change in dotcom first?)

/cc @primer/css-reviewers

Port the styles from dotcom to Primer.
@changeset-bot
Copy link

changeset-bot bot commented Sep 22, 2021

🦋 Changeset detected

Latest commit: ee6c52e

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 Minor

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

@talum talum marked this pull request as ready for review September 22, 2021 18:25
@talum talum requested a review from a team as a code owner September 22, 2021 18:25
@talum talum requested a review from vdepizzol September 22, 2021 18:25
}

:target {
border: $border-width $border-style var(--color-accent-emphasis);
Copy link
Contributor

@vdepizzol vdepizzol Sep 24, 2021

Choose a reason for hiding this comment

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

I'm noticing quite a jump in the footnote element when it receives the :target pseudo-class:

gif

  • Can we make sure the :target styling doesn't affect the position of the bullet point element?
  • Can we position the borders in such a way it that they don't touch the edges? See example below.
  • Should we be specific about this :target rule? It's currently being applied to any element inside .markdown-body .footnotes, but maybe we should be more specific.

Example of surrounding border better respecting the spacing around:

Screen Shot 2021-09-24 at 13 19 47
Link to Figma exploration

Thank you! 🙇 🙇 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense...I think this is going to be as close as I can get due to time constraints. I'll play around a little more on Monday and push it up. Thanks for your guidance and patience! I haven't done my own CSS in quite a bit of time. ✨
Screen Shot 2021-09-24 at 6 14 45 PM

Eliminated bullet point positioning jump
footnotes_css

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit tricky because the list-item numbers are "outside" of the <li>. Tried using list-style-position: inside;, but that caused problems with multi-line content like pagraphs.

This PR #1624 uses a ::before pseudo element to "draw" the :target style with negative positioning so it looks like the list items get wrapped:

Screen Shot 2021-09-27 at 17 35 00

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I like it!!! Yeah, I was trying a similar technique in this branch. It did made the multi-line content strange, which I tried to get around by adding margin-left to any paragraph that wasn't the first child.

I'm wondering if the right side should also have some positioning so that there's symmetry. Let me see.

Thank you for your help 🙏🏼 . The ::before pseudo element is interesting. I had tried to remove the jumpiness by adding a transparent border.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cherry-picked @simurai's commits. 🍒 Updated description ☝🏼 .

Also added the darkened text when targeted.

I didn't change the spacing between paragraphs (that's in the mock). It seemed really tight to me and made it hard to read.

Example:

Screen Shot 2021-09-27 at 9 19 05 AM

src/markdown/footnotes.scss Outdated Show resolved Hide resolved
@simurai simurai mentioned this pull request Sep 27, 2021
2 tasks
simurai and others added 5 commits September 27, 2021 09:02
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.

Tested it locally and looks great. ✨

Added a comment about using data-footnote-ref, but not sure if it's a big problem if we don't.

Comment on lines +99 to +105
sup > a::before {
content: "[";
}

sup > a::after {
content: "]";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chance that a user would want to add a link inside a <sup> and not get it wrapped with []? It seems we currently use the data-footnote-ref attribute. So if we wanted to scope this a bit more, we might can instead use:

Suggested change
sup > a::before {
content: "[";
}
sup > a::after {
content: "]";
}
[data-footnote-ref]::before {
content: "[";
}
[data-footnote-ref]::after {
content: "]";
}

Not sure if data-footnote-ref will be part of, I think we use a "common mark" library and safe to use? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm...That's a good question.

Yeah, data-footnote-ref is safe to use (I added it to the library 😄 ).

I think it's a pretty low chance that a user would want that behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, we can keep it "as is" and in case we hear reports from users, switch to using [data-footnote-ref].

Copy link
Contributor

@vdepizzol vdepizzol 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!

@simurai simurai enabled auto-merge (squash) September 29, 2021 03:01
@simurai simurai merged commit afac04b into primer:main Sep 29, 2021
@primer-css primer-css mentioned this pull request Sep 29, 2021
@simurai simurai mentioned this pull request Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants