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

Manage discord spoilers #3

Closed
wants to merge 6 commits into from
Closed

Conversation

Sorunome
Copy link
Contributor

The existing code for some {{spoiler}} doesn't seem to be what discords markdown for this is.

As of the point of writing this they introduced some ||spoiler|| with all kind of cases. Currently webbrowser discord just gobbles up the |, but android discord makes it a spoiler correctly.

This makes spoilers have the ability to have a custom callback, as, well, each environment may want to treat spoilers differently

@Sorunome Sorunome mentioned this pull request Jan 31, 2019
@brussell98
Copy link
Owner

Markdown on all clients will eventually be consistent as features are developed, so callback handling is not needed. Also, the library should not have custom html tags, so please revert it to the original output with altered regex.

@Sorunome
Copy link
Contributor Author

Also, the library should not have custom html tags, so please revert it to the original output with altered regex.

I am confused what you mean by this. the <spoiler> stuff was just for easy visualisation, it could also be :spoiler:

@brussell98
Copy link
Owner

brussell98 commented Jan 31, 2019

It should return like this return htmlTag('span', output(node.content, state), { class: 'd-spoiler' }, state);, not <spoiler>node.content</spoiler>

All elements use a span with a class for styling and support for css modules

@Sorunome
Copy link
Contributor Author

Sorunome commented Jan 31, 2019

it that case it won't be usable in systems that use a limited subset of html, as mentioned in #4, output customizability for such non-standard things are kinda needed there

@brussell98
Copy link
Owner

brussell98 commented Jan 31, 2019

I don't understand what you mean by that. The Pr must maintain consistency with the rest of the library. It is not my intention to support clients that have rare use-cases such as not being able to use classes.

If you want to patch the spoiler regex and test, that's fine. This PR introduces a breaking change by changing how we output tags and breaking CSS module support. It also includes and undocumented change to run eslint, which should at least be mentioned.

@Sorunome
Copy link
Contributor Author

while i adapted it now we will have to fork it, sadly

@brussell98
Copy link
Owner

Due to the number of issues with this PR I've decided to just implement it myself

@brussell98 brussell98 closed this Jan 31, 2019
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