Navigation Menu

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

Hashtag linking improvements #43

Merged
merged 2 commits into from Feb 1, 2019
Merged

Hashtag linking improvements #43

merged 2 commits into from Feb 1, 2019

Conversation

mrvdb
Copy link
Collaborator

@mrvdb mrvdb commented Dec 3, 2018

This improves rendering in a number of situations:

  • it keeps anchor tags working
  • it gives the user some control for not linking, for example in code
    blocks.

Con:
hashTags at the beginning of a line without a space won't get linked.

Workaround related to issues #42 and #6 and #33

This improves rendering in a number of situations:

- it keeps anchor tags working
- it gives the user some control for not linking, for example in code
  blocks.

Con:
hashTags at the beginning of a line without a space won't get linked.

Workaround related to issues #42 and #6 and #33
@thebaer
Copy link
Member

thebaer commented Dec 4, 2018

Thanks for trying to fix this. I agree this would solve some problems (especially for heavy users of same-page anchor tags), but breaking tags at the beginning of a line is too much of a regression to merge this. It also breaks tags that are put together without spaces around them, e.g. only 3 out of 8 tags correctly render in a post like this:

#test #lots #of #hashtags
#many#tags#every#where

I'm fine with suboptimal fixes sometimes, but especially since:

  • it's currently possible to work around the in-page anchor issues, and
  • this would introduce a visible bug that fails in these two very common cases,

I'd rather keep the current shortcomings until we have a fix for all scenarios mentioned in those issues.

@mrvdb
Copy link
Collaborator Author

mrvdb commented Dec 4, 2018

Fair enough. Although, your second example line, is that really a common case?

The beginning of line failure is indeed a worry. I haven't really tried hard to get that into the regex, which could probably work, using the same method as getting the 'space case' to work.

For the generic case, having a working parser for a non-standardized format, as you mention 'we're not a markdown only shop', will be quite a challenge I think.

@mrvdb
Copy link
Collaborator Author

mrvdb commented Dec 4, 2018

FYI I'm keeping a test page up here for myself which has the changes in this branch: https://qua.name/debug/test-for-hashlink

@mrvdb mrvdb changed the title Require hashtags to have a space before them Hashtag linking improvements Dec 4, 2018
@thebaer
Copy link
Member

thebaer commented Dec 17, 2018

Sorry I dropped off the map on this one. I agree the second case isn't common, but I'd like to support it, as I think it's expected behavior for anyone coming from other social networks, like Instagram.

I appreciate you taking the time on this. It's a good temporary fix, but as this will be deployed in an attempt to fix the issue for all WF users, including 40k+ users on Write.as, I'd really like to implement a permanent fix, likely involving changes to the saturday parser similar to the ones mentioned in #6. I believe it'll be straightforward, especially after figuring out how the parser works. So I'm gonna try to get to it this week.

Copy link
Member

@thebaer thebaer left a comment

Choose a reason for hiding this comment

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

Issues should be fixed with changes to the Markdown parser, where it's aware of when a "hashtag" is inside a href attribute or code block.

@mrvdb
Copy link
Collaborator Author

mrvdb commented Dec 17, 2018

No worries. I wanted to have the temporary fixes published somewhere so other instances could choose to patch theirs if they wanted to. I fully agree the permanent fix needs to be different.
Wouldn't it help if we were coding against a markdown standard like https://commonmark.org or is that a bridge too far?

@thebaer
Copy link
Member

thebaer commented Dec 17, 2018

I don't know that it'd help, since we'll have to extend the standard no matter what. Or is there some other way you were thinking it'd help out?

Either way, I could see the case for adding the option between a standard like commonmark or our restricted flavor of Markdown in the future.

@mrvdb
Copy link
Collaborator Author

mrvdb commented Dec 17, 2018

I was indeed advocating conforming to *a* standard indeed; commonmark seemed the most sensible.

If that needs to be in the form of an option for compatibility reasons, that is fine obviously

@thebaer
Copy link
Member

thebaer commented Dec 17, 2018

Yeah, I want to support users who just want to write in plain text, and I think the current library is a decent tradeoff between supporting that and basic Markdown without requiring any explicit customization.

But I've added this as a new task: T557

@thebaer
Copy link
Member

thebaer commented Jan 26, 2019

I'm going to take another crack at the parser-based fix this coming week, but if I'm not able to make progress, I'll merge this in. Since this fixes so many different issues, and hashtags at the beginning of a line work, I think this will be a good solution.

@thebaer
Copy link
Member

thebaer commented Feb 1, 2019

Merging now -- thanks again!

Closes #33, closes #42

@thebaer thebaer merged commit ee6046b into writefreely:master Feb 1, 2019
@thebaer thebaer mentioned this pull request Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants