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

Add material icons for file list #33837

Merged
merged 3 commits into from
Mar 10, 2025
Merged

Add material icons for file list #33837

merged 3 commits into from
Mar 10, 2025

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 9, 2025 •
edited
Loading

This one should be good enough.

Doc: https://gitea.com/gitea/docs/pulls/181

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 9, 2025
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 9, 2025
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 9, 2025 •
edited
Loading

  • Added material-icon-theme as a dev dependency, and generate the icons by our script.
  • The SVG icons are managed in a JSON file, it will be compressed when packing into the binary, so the size is acceptable (about 270KB).
  • Used some tricks to make the SVG icon work well for rendering, could be refactored in the future.

image

@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/dependencies modifies/frontend docs-update-needed The document needs to be updated synchronously labels Mar 9, 2025
@wxiaoguang wxiaoguang mentioned this pull request Mar 9, 2025
Closed
@wxiaoguang wxiaoguang force-pushed the custom-file-icon-2 branch from c1f191a to 318a317 Compare March 9, 2025 17:35
@wxiaoguang wxiaoguang added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Mar 9, 2025
@wxiaoguang wxiaoguang force-pushed the custom-file-icon-2 branch 2 times, most recently from 773ec63 to 7990e65 Compare March 9, 2025 18:02
@wxiaoguang wxiaoguang added this to the 1.24.0 milestone Mar 9, 2025
@wxiaoguang wxiaoguang force-pushed the custom-file-icon-2 branch 2 times, most recently from 12703f4 to e240070 Compare March 9, 2025 18:36
@wxiaoguang wxiaoguang force-pushed the custom-file-icon-2 branch from e240070 to b23f073 Compare March 9, 2025 18:38
@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 Mar 9, 2025
@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 Mar 10, 2025
@wxiaoguang
Copy link
Contributor Author

Let's merge and try it. If there is any problem, there are chances to revert or improve in following PRs.

@wxiaoguang wxiaoguang merged commit 34e5df6 into go-gitea:main Mar 10, 2025
26 checks passed
@wxiaoguang wxiaoguang deleted the custom-file-icon-2 branch March 10, 2025 07:57
@wxiaoguang wxiaoguang mentioned this pull request Mar 10, 2025
Closed
@wxiaoguang
Copy link
Contributor Author

Follow up fix: Fix material icon & diff highlight #33844

@lunny lunny mentioned this pull request Mar 10, 2025
Draft
3 tasks
hiifong pushed a commit to hiifong/gitea that referenced this pull request Mar 10, 2025
@wxiaoguang @hiifong
Add material icons for file list (go-gitea#33837)
55f1605 
(cherry picked from commit 34e5df6)
@delvh
Copy link
Member

delvh commented Mar 10, 2025

Small question: What exactly changed since the last proposal by @yardenshoham?
I do think I remember concerns about resource usage and being "too colorful" which led to the original proposal being blocked.
Don't get me wrong, I'm absolutely in favor of finally having some more icons.
But what exactly is different now?

@delvh
Copy link
Member

delvh commented Mar 10, 2025

Okay, I just skimmed over the changes.
The answer seems to be that it incorporates exactly these points 👍

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 12, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
62e84f3 
* giteaofficial/main:
  Fix various Fomantic UI and htmx problems (go-gitea#33851)
  Add workflow_job webhook (go-gitea#33694)
  Fix file icon mapping (go-gitea#33855)
  Drop fomantic build (go-gitea#33845)
  Fix auto concurrency cancellation skips commit status updates (go-gitea#33764)
  Fix test code (go-gitea#33829)
  Remove "noscript" tag from html head (go-gitea#33846)
  Fix material icon & diff highlight (go-gitea#33844)
  Fix LFS URL (go-gitea#33840)
  Add material icons for file list (go-gitea#33837)
lutinglt
lutinglt reviewed Mar 14, 2025
View reviewed changes
templates/repo/view_list.tmpl
@@ -26,7 +26,6 @@
{{else}}
{{if $entry.IsDir}}
{{$subJumpablePathName := $entry.GetSubJumpablePathName}}
{{svg "octicon-file-directory-fill"}}
Copy link

Choose a reason for hiding this comment

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

Have you considered not removing this identifier for compatibility with other icon plugins, such as Catppuccin for GitHub File Explorer Icons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH I don't know how these plugins work. And Gitea doesn't promise HTML layout / template compatibility between release. For example: before this PR, the "file list" has been heavily refactored in GitHub like repo home page #32213

If you have ideas about how to improve the layout and/or make plugins work, feel free to make some proposals/PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just read the plugin's code briefly.

If you mean that "we need to add a special CSS name for directories to make it could be query-selected by the plugin", I think we can add it in renderFileIconSVG function, it should be able to know whether the current entry is a dir or a file.

Copy link

Choose a reason for hiding this comment

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

You can make changes as you like, or ignore this review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry but I haven't got your point .....

Do you mean that it should keep octicon-file-directory-fill? Or it could output something like this?

<svg class="svg ... icontype-dir">for a directory</svg>
<svg class="svg ...">for a file</svg>

?

Copy link

Choose a reason for hiding this comment

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

Yes, my opinion is that there is no need to modify this class attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-> Add old svg class name to git entry icon #33884

@wxiaoguang wxiaoguang mentioned this pull request Mar 14, 2025
Merged
wxiaoguang added a commit that referenced this pull request Mar 14, 2025
@wxiaoguang
Add old svg class name to git entry icon (#33884)
0dfa94e 
Fix #33837 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lutinglt lutinglt lutinglt left review comments

@lunny lunny lunny approved these changes

@yardenshoham yardenshoham yardenshoham approved these changes

@hiifong hiifong hiifong approved these changes

Assignees
No one assigned
Labels
docs-update-needed The document needs to be updated synchronously lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/dependencies modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.24.0
Development

Successfully merging this pull request may close these issues.
7 participants
@wxiaoguang @delvh @lunny @yardenshoham @hiifong @lutinglt @GiteaBot