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

Switch syntax highlighting to PrismJS #3685

Closed
wants to merge 4 commits into from
Closed

Switch syntax highlighting to PrismJS #3685

wants to merge 4 commits into from

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Mar 18, 2018

Looks to be working fine except for diff views that still needs to be fixed. Would need some help on testing more source code to see if it does not break.

Fixes #3682

@lafriks lafriks added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Mar 18, 2018
@codecov-io
Copy link

codecov-io commented Mar 18, 2018 •
edited
Loading

Codecov Report

Merging #3685 into master will decrease coverage by 0.01%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3685      +/-   ##
==========================================
- Coverage    35.8%   35.79%   -0.02%     
==========================================
  Files         287      287              
  Lines       41489    41489              
==========================================
- Hits        14857    14852       -5     
- Misses      24445    24449       +4     
- Partials     2187     2188       +1
Impacted Files Coverage Δ
modules/markup/sanitizer.go 100% <100%> (ø) ⬆️
modules/highlight/highlight.go 31.81% <25%> (ø) ⬆️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/repo_list.go 56.37% <0%> (-1.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c28bf94...a604870. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 18, 2018
@lafriks lafriks closed this Mar 18, 2018
@lafriks lafriks reopened this Mar 18, 2018
@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 18, 2018
lunny
lunny requested changes Mar 18, 2018
View reviewed changes
modules/highlight/highlight.go
".mlx": "matlab",
".pas": "pascal",
".patch": "diff",
".py": "python",
Copy link
Member

Choose a reason for hiding this comment

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

There is no .go support?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is, just in other dict

Copy link

Choose a reason for hiding this comment

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

Please add D

@lafriks lafriks force-pushed the feat/syntax_highlight_prism branch from 8aa56a8 to a604870 Compare March 18, 2018 22:20
@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 18, 2018
@lafriks
Copy link
Member Author

lafriks commented Mar 18, 2018

I have fixed diff view but currently changed line part highlight is still lost

@lafriks
Copy link
Member Author

lafriks commented Apr 29, 2018

In my testing for large files performance is awful so we should probably look at server side rendering

@lafriks lafriks added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Apr 29, 2018
@techknowlogick techknowlogick added this to the 1.6.0 milestone May 26, 2018
@lafriks lafriks modified the milestones: 1.6.0, 1.x.x Jul 4, 2018
@sysint64
Copy link

It's looks like #1337 will also be fixed with this PR

@Tyil
Copy link

Tyil commented Dec 29, 2018

Any progress on this, or is the performance bad enough to instead consider Chroma as suggested in #3682 (comment)? I would like to improve support for Perl 6 syntax highlighting in Gitea, so I would like to know if I should look into adding it to PrismJS or Chroma.

@lafriks
Copy link
Member Author

lafriks commented Dec 30, 2018

That has to be tested

@techknowlogick
Copy link
Member

Closing due to performance reasons, and also because we will be switching to server side rendering (likely using chroma)

@lafriks lafriks removed this from the 1.x.x milestone Jan 24, 2019
@GitMensch
Copy link

Closing [...] because we will be switching to server side rendering (likely using chroma)

Reference: #7729.

@lafriks lafriks deleted the feat/syntax_highlight_prism branch June 8, 2020 09:09
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sysint64 sysint64 sysint64 left review comments

@lunny lunny lunny approved these changes

@appleboy appleboy appleboy approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/wip This PR is not ready for review status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JavaScript syntax highlighting breaks
9 participants
@lafriks @codecov-io @sysint64 @Tyil @techknowlogick @GitMensch @appleboy @lunny @tboerger