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: support mapping on custom meta tag #1142

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

simoneb
Copy link

@simoneb simoneb commented Aug 9, 2023

This introduces support for specifying a custom meta tag to get the discussion title from, because in some cases none of the options available is suitable.

This introduces support for specifying a custom meta tag to get the discussion title from, because in some cases none of the options available is suitable.
@vercel
Copy link

vercel bot commented Aug 9, 2023

@simoneb is attempting to deploy a commit to the giscus Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

Thanks, but if this were to be added officially, we'll need to add it to the configuration interface as well.

You can also self-host just the client script and make these changes, though.

@simoneb
Copy link
Author

simoneb commented Aug 13, 2023

What option would you prefer? I think the feature may be useful to others, so if you could point me to the right place where those changes need to be done I'd be glad to do them.

Overall though, I believe a more general-purpose way to load the title of the discussion from any place would be ideal, so you don't have to come up with all possible places it could come form

@laymonage
Copy link
Member

That's a good idea, yeah I think it's better to provide a generic way to customise the mapping. Maybe a custom event can be fired by the script, then if you add an event listener for that event, you can tell it what the mapping term should be. Then #854 can also be reworked to use a similar custom event approach.

That said, I think it'd be better to rework the client script to use the web component before doing this, so that I don't have to maintain two different yet similar things...

@simoneb
Copy link
Author

simoneb commented Aug 20, 2023

@laymonage how would you like to go about this? Do you want to take it up yourself or get a contribution?

@laymonage
Copy link
Member

I wouldn't mind either way. Unfortunately I don't have much free time these days though, so I won't be doing it any time soon and I don't think I'd be able to give much guidance for contributors. Anyway, I'm thinking the steps would be to:

  1. Convert client.ts so that it only acts as a wrapper that passes its data-* attributes to the web component's attributes.
    • Not sure how the web component should be loaded, though. I don't know whether it can be directly imported in client.ts or whether it should dynamically create a <script> tag that loads the web component using <script type="module".
  2. Make changes to the web component's code so that it can listen to a custom event to get the term.
    • This might not be easy as we'll likely need to delay the iframe from loading until the term is acquired. Could probably make a new mapping option for this though, and only delay the load if this mapping e.g. mapping="event" is used.
  3. Update the Configuration.tsx component so that there's another option to make the mapping rely on the custom event.
  4. Update the documentation describing how to use a custom event to set the mapping.

That said, I'm still not sure whether the component should listen for the event e.g. giscus:setTerm and your code fire the event to the component, or should it be the other way around i.e. the component fires an event when it needs the term, and you should listen to this event and provide it via the callback somehow.

@simoneb
Copy link
Author

simoneb commented Aug 28, 2023

Okay understood, it looks like going generic is a considerable undertaking and implies a level of complexity that the current approach doesn't have. How about we stick with my proposed solution which allows loading it from any meta tag, and then do the relevant changes to the Config and docs?

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