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.

Sign up for GitHub

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

Jump to bottom

remove comments spellcheck for amp validation #1106

Merged
merged 1 commit into from
Oct 21, 2017
Merged

remove comments spellcheck for amp validation #1106

merged 1 commit into from
Oct 21, 2017

Conversation

emmanuelgautier
Copy link
Contributor

spellcheck attribute cause AMP validation errors. This pull request remove this optional attribute to allow prismjs preprocess AMP pages code.

@Golmote
Copy link
Contributor

Golmote commented Sep 9, 2017

Hi @emmanuelgautier. Is there any way we could keep when not on AMP pages? Like, could we detect we're on an AMP page and decide to not add it then?

@emmanuelgautier
Copy link
Contributor Author

Hi @Golmote, highlighting is processing on server side so it could be possible to imagine an option for the highlight function. It also possible to detect that the tag <html ⚡> or this one <html amp> is present. I think the best way is to give function the capability to know his running context.

What do you think about this two solutions ?

@Golmote
Copy link
Contributor

Golmote commented Oct 19, 2017

I'd rather add it as a global option for Prism, the kind we have for manual highlighting. Something in the lines of:

<script>
var Prism = {
    spellcheck: false
};
</script>
<script src="prism.js"></script>

See #1087 and #1188

@mAAdhaTTah
Copy link
Member

Why do comments need spellcheck="true"?

@Golmote
Copy link
Contributor

Golmote commented Oct 19, 2017

I actually never though about this... I guess @LeaVerou tought it would be convenient. This has been around for quite some time I believe. Now that I think about it, it only makes sense with contenteditable="true", so it must have been used initially in Dabblet.

@mAAdhaTTah
Copy link
Member

There is a _.hooks.run('wrap', env); right after the env gets built. If this is needed by Dabblet, it should be doable to hook into the env from outside and add spellcheck="true" if needed by Dabblet or anyone else. I'm 👍 for going ahead and removing, unless @LeaVerou has objections due to conflicts in Dabblet.

@LeaVerou
Copy link
Member

I’m fine with removing it. :)
I don't think we need to add a pre-init option for it, as @mAAdhaTTah pointed out, there's a hook if someone needs it desperately.

@Golmote Golmote merged commit 01e74af into PrismJS:master Oct 21, 2017
@oliviertassinari
Copy link
Contributor

🎉

@emmanuelgautier emmanuelgautier deleted the remove_spellcheck_attribute branch February 2, 2018 09:05
Rob--W added a commit to Rob--W/crxviewer that referenced this pull request Dec 10, 2018
@Rob--W
Stop deleting obsolete spellcheck option in Prism
1223cd1 
The attribute is no longer set by Prism:
PrismJS/prism#1106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@emmanuelgautier @Golmote @mAAdhaTTah @LeaVerou @oliviertassinari