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

Server-side syntax highlighting for all code #12047

Merged
merged 23 commits into from
Jun 30, 2020
Merged

Server-side syntax highlighting for all code #12047

merged 23 commits into from
Jun 30, 2020

Conversation

mrsdizzie
Copy link
Member

@mrsdizzie mrsdizzie commented Jun 24, 2020 •
edited
Loading

This PR does a few things:

  • Remove all traces of highlight.js
  • Use chroma library to provide fast syntax highlighting directly on the server
  • Provide syntax highlighting for diffs + code comments
  • Re-style code view and unified/split diffs views to try and look a bit more 'modern'
  • Add custom syntax highlighting styling for regular theme and convert previous arc-green highlighting best I could

Fixes #7729
Fixes #10157
Fixes #11825
Fixes #7728
Fixes #3872
Fixes #3682

And perhaps gets closer to #9553

Suggestions welcome for styling

New Diff:

Screen Shot 2020-06-24 at 4 13 12 PM

Screen Shot 2020-06-24 at 7 31 29 PM

Screen Shot 2020-06-24 at 7 46 21 PM

Screen Shot 2020-06-24 at 7 43 10 PM

Screen Shot 2020-06-26 at 12 26 37 PM

@mrsdizzie
Server-side syntax hilighting for all code
811678c 
This PR does a few things:

* Remove all traces of highlight.js
* Use chroma library to provide fast syntax hilighting directly on the server
* Provide syntax hilighting for diffs
* Re-style both unified and split diffs views
* Add custom syntax hilighting styling for both regular and arc-green

Fixes go-gitea#7729
Fixes go-gitea#10157
Fixes go-gitea#11825
Fixes go-gitea#7728
Fixes go-gitea#3872
Fixes go-gitea#3682

And perhaps gets closer to go-gitea#9553
@mrsdizzie
fix line marker
03c948c
@mrsdizzie
Merge branch 'master' into chroma-highlight
d9dc858
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Jun 25, 2020
@lafriks lafriks added this to the 1.13.0 milestone Jun 25, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 25, 2020
@silverwind
Copy link
Member

I think you still need to run npm uninstall highlight.js and commit the results.

@mrsdizzie
Copy link
Member Author

I think you still need to run npm uninstall highlight.js and commit the results.

Thanks! fixed

@silverwind
Copy link
Member

Oh, and npm uninstall domino as well and remove this line.

@mrsdizzie
Copy link
Member Author

Oh, and npm uninstall domino as well and remove this line.

fixed

@GiteaBot GiteaBot 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 Jun 29, 2020
@mrsdizzie
Copy link
Member Author

@silverwind made a few tweaks to light theme based on the link you gave above -- think it is pretty good for now. Pygments doesn't have quite the level of detail that pretty lights does in some areas but is pretty close.

Also raised the threshold for highlighting to 1MB based on https://help.github.com/en/github/creating-cloning-and-archiving-repositories/limits-for-viewing-content-and-diffs-in-a-repository

@silverwind
Copy link
Member

Will likely need to undo to worker-loader removal (part of 901ce59) because #12095 will depend on it.

@mrsdizzie
Copy link
Member Author

Will likely need to undo to worker-loader removal (part of 901ce59) because #12095 will depend on it.

Do I need to do any special commands to properly add it back to npm, or just add the line back to package.json and run make frontend?

@silverwind
Copy link
Member

Run npm install worker-loader and add back the two removed sections in webpack.config.js and .eslintrc. npm install will take care of package-lock.json.

@GiteaBot GiteaBot 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 Jun 30, 2020
@silverwind
Copy link
Member

Syntax looks good on light and dark. I have not encountered some of the more exotic identifiers in the files I was looking at but we should be good with those brighter color on arc-green.

@lafriks
Copy link
Member

lafriks commented Jun 30, 2020

🚀

@lafriks lafriks merged commit af7ffaa into go-gitea:master Jun 30, 2020
@mrsdizzie
Copy link
Member Author

Thanks all : )

This was referenced Jul 7, 2020
Closed
Closed
This was referenced Jul 7, 2020
Closed
Closed
mrsdizzie added a commit to mrsdizzie/gitea that referenced this pull request Jul 8, 2020
@mrsdizzie
Make copy/paste work for source code
4620c82 
Fix regression casued by go-gitea#12047 so copy/paste works properly in all browsers.

Fixes go-gitea#12184

Also while looking at this I saw a small display issue for blame view. I think go-gitea#12023 was merged into original PR through an update branch before go-gitea#12047 was merged and made one of the css ruules not apply anymore.
@mrsdizzie mrsdizzie mentioned this pull request Jul 8, 2020
Merged
zeripath pushed a commit that referenced this pull request Jul 8, 2020
@mrsdizzie
Make copy/paste work for source code (#12191)
a6168fa 
* Make copy/paste work for source code

Fix regression casued by #12047 so copy/paste works properly in all browsers.

Fixes #12184

Also while looking at this I saw a small display issue for blame view. I think #12023 was merged into original PR through an update branch before #12047 was merged and made one of the css ruules not apply anymore.

* use pseudo-element to prevent copying of comment + symbol even when not visually selected

* remove added newline here should not be necessary anymore

* make sure empty line is newline so there is something to select and copy
@silverwind silverwind mentioned this pull request Jul 24, 2020
Closed
5 tasks
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
@mrsdizzie @silverwind @lafriks
Server-side syntax highlighting for all code (go-gitea#12047)
845b9f1 
* Server-side syntax hilighting for all code

This PR does a few things:

* Remove all traces of highlight.js
* Use chroma library to provide fast syntax hilighting directly on the server
* Provide syntax hilighting for diffs
* Re-style both unified and split diffs views
* Add custom syntax hilighting styling for both regular and arc-green

Fixes go-gitea#7729
Fixes go-gitea#10157
Fixes go-gitea#11825
Fixes go-gitea#7728
Fixes go-gitea#3872
Fixes go-gitea#3682

And perhaps gets closer to go-gitea#9553

* fix line marker

* fix repo search

* Fix single line select

* properly load settings

* npm uninstall highlight.js

* review suggestion

* code review

* forgot to call function

* fix test

* Apply suggestions from code review

suggestions from @silverwind thanks

Co-authored-by: silverwind <me@silverwind.io>

* code review

* copy/paste error

* Use const for highlight size limit

* Update web_src/less/_repository.less

Co-authored-by: Lauris BH <lauris@nix.lv>

* update size limit to 1MB and other styling tweaks

* fix highlighting for certain diff sections

* fix test

* add worker back as suggested

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Lauris BH <lauris@nix.lv>
@richmahn richmahn mentioned this pull request Sep 14, 2020
Closed
7 tasks
@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

@lafriks lafriks lafriks approved these changes

@silverwind silverwind silverwind 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. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.13.0
4 participants
@mrsdizzie @silverwind @lafriks @GiteaBot