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

Support arbitrary permalink attributes #63

Merged
merged 6 commits into from
Feb 5, 2020

Conversation

oliverjam
Copy link
Contributor

As discussed in #62, it would be nice to give the user the ability to set extra HTML attributes on the permalink anchor.

  • Adds a permalinkAttrs object to opts that sets all key/value pairs at attributes
  • Tests
  • Docs

@oliverjam
Copy link
Contributor Author

@nagaozen not sure if you saw this, but what do you think?

@nagaozen
Copy link
Collaborator

I'm not sure if this is the right approach to the problem. Could you imagine another use case for this option? Otherwise we could just observe for permalink href and inject aria attributes automatically

@oliverjam
Copy link
Contributor Author

There are lots of HTML and ARIA attributes that devs might want to set on the link. For example: tabindex or title. This library currently only allows setting the className, which isn't as flexible.

Injecting the aria-label automatically only works if the dev doesn't want to customise it to have more or less information than we decide. Some people might be happy with the label just being "permalink", whereas others might want to use the text content of the heading, or put custom text in there to describe what exactly the permalink is for.

@nagaozen
Copy link
Collaborator

So, I believe that we are looking for a function which also receives (slug, state) and returns an object used to populate custom attributes to the anchor... could you fix the PR to receive a function instead an object? This way the behaviour is far more dynamic and the option becomes more useful.

@oliverjam
Copy link
Contributor Author

I agree, that's a great idea, I'll add it asap.

Would you just support a function, or could we fallback to an object for the simple case where a dev just wants to set (for example) a static label?

@nagaozen
Copy link
Collaborator

I believe just a function is better, no need to overload the option. We must try to keep it simple and coherent with the other options

Copy link
Collaborator

@nagaozen nagaozen left a comment

Choose a reason for hiding this comment

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

PR is ready to merge

@nagaozen nagaozen merged commit 3acc028 into valeriangalliat:master Feb 5, 2020
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.

2 participants