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

More dvc.org implementation fixes #140

Merged
merged 5 commits into from
Nov 30, 2022
Merged

More dvc.org implementation fixes #140

merged 5 commits into from
Nov 30, 2022

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Nov 29, 2022

For use with iterative/dvc.org#4118, which will depend on this being merged and shipped to fix some issues.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tonistiigi Tõnis Tiigi

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tonistiigi Tõnis Tiigi
@rogermparent rogermparent self-assigned this Nov 29, 2022
Comment on lines 37 to +38
h1:first-child {
@apply lg:h-14 pb-2;
@apply pb-2 min-h-[3.5rem];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

min-h over h fixes an issue where line broken titles were off

Comment on lines 1 to 4
* {
@apply scroll-mt-14;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scroll margin now is on the header, since it's pretty much coupled. We use Tailwind's 14 (3.5rem) but this makes me think we may want to specify a tailwind custom value.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tonistiigi Tõnis Tiigi

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tonistiigi Tõnis Tiigi
@@ -44,6 +44,7 @@
h4,
h5,
h6 {
@apply scroll-mt-5;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

scroll-pt-top on the body makes up for the sticky header exactly, and elements that want to have a little extra space can set scroll-mt to be used in addition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice addition. I think we can add them to other linkers as well like details link, args link etc which also provide hash links beside headers.

@rogermparent rogermparent marked this pull request as ready for review November 30, 2022 04:24
@rogermparent rogermparent requested a review from a team as a code owner November 30, 2022 04:24
Copy link
Contributor

@yathomasi yathomasi left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -44,6 +44,7 @@
h4,
h5,
h6 {
@apply scroll-mt-5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice addition. I think we can add them to other linkers as well like details link, args link etc which also provide hash links beside headers.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
tonistiigi Tõnis Tiigi
@yathomasi yathomasi merged commit 4fbe9b9 into main Nov 30, 2022
@yathomasi yathomasi deleted the fix-fixed-md-headers branch November 30, 2022 05:53
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.

None yet

2 participants