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

feat: added # button to heading #155

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

heydoyouknowme0
Copy link
Contributor

No description provided.

@heydoyouknowme0 heydoyouknowme0 linked an issue Apr 24, 2024 that may be closed by this pull request
@heydoyouknowme0 heydoyouknowme0 changed the title added # button to heading feat: added # button to heading Apr 24, 2024
Copy link
Member

@GetPsyched GetPsyched left a comment

Choose a reason for hiding this comment

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

2 issues:

  • Should be a ease-in-out animation with some gap between the hash and text
  • Hovering on the glyphs should also trigger the hash button

GetPsyched

This comment was marked as outdated.

Copy link
Member

@GetPsyched GetPsyched left a comment

Choose a reason for hiding this comment

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

I don't think content based is good. It's usually avoided due to it being hacky. But feel free to use it if you fix this:
image

@heydoyouknowme0
Copy link
Contributor Author

its firefox again, I could have just used a span(dont know which is better semantically)
but I need something to provide that padding in between, cannot use padding in parent in cause it will take space in the parent container, margin/padding in link also causes undesirable behaviours. content disappears from the dom when hidden feels the most elegant here

@GetPsyched GetPsyched force-pushed the feat/heading-button branch from 4afd62c to 9b30c78 Compare May 6, 2024 01:13
Copy link
Member

@GetPsyched GetPsyched left a comment

Choose a reason for hiding this comment

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

Some styling fixes remain

@heydoyouknowme0
Copy link
Contributor Author

Some styling fixes remain

you removed the comp from the button, it there any problem with that cause that was the best fix to the problem that got introduced now

@GetPsyched
Copy link
Member

<Comp> in the code corresponds to the heading we're using. It doesn't feel right to have <h2> for the heading and then another <h2> for a button?

@GetPsyched
Copy link
Member

Uh, @heydoyouknowme0?

@heydoyouknowme0
Copy link
Contributor Author

heydoyouknowme0 commented May 9, 2024

I've spent a lot of time on this, using comp or adding a span(or something to encompass it, then styling it) is the only way I could find to make it work as intended.

Latest code still has problems with making the gap unclickable which weren't present before
IMG_20240509_160912338_HDR

@GetPsyched
Copy link
Member

If we want to make the gap unclickable we can just add a margin instead of a padding, no?

@heydoyouknowme0
Copy link
Contributor Author

Nope then hover isn't registered

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.

Show button to get heading link
2 participants