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

emojis in titles not working correctly #1009

Closed
1 task done
theoephraim opened this issue Jan 21, 2020 · 13 comments · Fixed by #1016 or #1746
Closed
1 task done

emojis in titles not working correctly #1009

theoephraim opened this issue Jan 21, 2020 · 13 comments · Fixed by #1016 or #1746
Labels

Comments

@theoephraim
Copy link

theoephraim commented Jan 21, 2020

Bug Report

Processing the emojis in titles isn't working correctly. Sometimes just turning it into a single ":" character. Seems to work correctly if no leading space before the emoji code.

Steps to reproduce

Insert this content in a page

#### :arrow_forward:  :arrow_forward:
#### :arrow_forward::arrow_forward:
####  :arrow_forward: :arrow_forward:
####  :arrow_forward::arrow_forward:
####  :arrow_forward: :arrow_forward:

What is current behaviour

image

What is the expected behaviour

All emojis should appear

Other relevant information

  • Bug does still occur when all/other plugins are disabled?

  • Your OS: mac

  • Node.js version: 11.15

  • npm/yarn version: 6.7

  • Browser version: chrome latest

  • Docsify version: cli is 4.4, using latest from cdn

  • Docsify plugins: all disabled

@theoephraim theoephraim changed the title emoji at beginning of title not working emojis in titles not working correctly Jan 21, 2020
@anikethsaha anikethsaha added bug confirmed as a bug help wanted labels Feb 3, 2020
@Koooooo-7
Copy link
Member

Koooooo-7 commented Feb 4, 2020

@anikethsaha
heyyyyy, I investigated this bug, but it seems complicated for me.
so, I wanna share u to make it fixed possiblely.
I guess the issue is the regex of the function getAndRemoveConfig at compile.js

/(?:^|\s):([\w-]+)=?([\w-]+)?/g

I cheked the history of the function.
FIRSTLY, this function was applied for the fix: zoom image plugin issue to get configurations about images. and it hadnt been called on the titles.

THEN, A new feature feat: add heading config id called the function for the titles to get configurations. At the same time, there is the bug that the regex worked correctly to the title.

NEXT, the regex was updated once to fix one case Fix getAndRemoveConfig regex likes # foo::bar::baz.

Although, I tried to rewrite the regex ( a little bit hard for me ) or just keep the original title to be rendered to fix the bug, I thought the function should be separate for different places, or in another way. beside that, the emojis have configuration noEmoji also.

@anikethsaha
Copy link
Member

@Koooooo-7 thanks a lot for detailed investigation
What I am thinking is,
as of now a regex to support all seem complicated, and it's hard for me as well to support the issues addressed here and in #708
I am proposing is to document this clearly that in order to support emojis, spaces are required at the beginning.
We cant revert the #708 changes as it will break the site for Perl and other similar syntax-based lang.

@Koooooo-7
Copy link
Member

@anikethsaha
yep, I agree to make it clearly that how to support emojis at titles.
but I afraid that there is a potential problem about the : of the title, configuration or emojis.

@anikethsaha
Copy link
Member

I afraid that there is a potential problem about the : of the title, configuration or emojis.

Can you mention some ?

@Koooooo-7
Copy link
Member

@anikethsaha
A. the date 2020:02:02 is problem as u know.
B. Some titles with different spaces likes :

### the news : my daily  :dog:
### the news :my daily  :dog:
### the news:my daily  :dog:
### the news: my daily  :dog:

All of them r not working correctly.

If possible, I think we need rewrite the feature feat: add heading config id in another way that we dont through the : to set and get configurations at the title.

@theoephraim
Copy link
Author

I don't think trying to document this behaviour is going to work since it seems pretty unpredictable (look closely at the examples above).

Here's an idea - for the weird code syntax cases that #708 was trying to fix, require users to put them within a code backticks to avoid any conflicting treatment from config or emojis, for example:

## `MyPerlModule::Submodule::function` :emoji: :id=something

@anikethsaha
Copy link
Member

another way that we dont through the : to set and get configurations at the title.

then what are you suggesting ? cause if you are thinking to consider other char, then I am pretty sure it will break other things or will give issues like #708 for other langs

@anikethsaha
Copy link
Member

Here's an idea - for the weird code syntax cases that #708 was trying to fix, require users to put them within a code backticks to avoid any conflicting treatment from config or emojis, for example:

## `MyPerlModule::Submodule::function` :emoji: :id=something

It will render as code block like this

MyPerlModule::Submodule::function :emoji: :id=something

It seems fine by me but I cant give assurance on behave of others.
@Koooooo-7 what do you think ?

@Koooooo-7
Copy link
Member

Koooooo-7 commented Feb 4, 2020

this case ## `MyPerlModule::Submodule::function` :emoji: :id=something is not working correctly for me right now.

if it will render as u posted above, I think it is okay at now, but Idk how to document this clearly .

I think we may use {} likes## Header {docsify-ignore} contain the configurations to rewrite that feature instead.

@anikethsaha
Copy link
Member

I think it wont render as I commented. it will be different. I need to look there.

I think we may use {} likes## Header {docsify-ignore} contain the configurations to rewrite that feature instead.

lets have triple {{{ }}} as I think it's safe. cause Php has {} and mustache template lib has {{ }} too I think.

Anything else you think will fit here better ?

@trusktr , what do you think ?

@Koooooo-7
Copy link
Member

I think the key is we can set the prefix to avoid conflict from others. such as {docsify-id-myid}.

@anikethsaha
Copy link
Member

I think the key is we can set the prefix to avoid conflict from others. such as {docsify-id-myid}.

+1 for this as well.

Note : any changes we make regarding this will be breaking change. I think it should be fit for 5.x

Lets have a PoC for this.
Feel free to submit PoC to tackle this

@anikethsaha
Copy link
Member

A better regex to support the current would be better if possible as it has fewer consequences of the change.

Feel free to submit the better regex as well if possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants