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

Improve RSS feed icons #28368

Merged
merged 2 commits into from
Dec 6, 2023
Merged

Improve RSS feed icons #28368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65aacec
Improve RSS feed icons
n0toose Dec 6, 2023
68df4e1
Merge branch 'main' into rss-feed-icon
GiteaBot Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions templates/org/home.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@
{{if .Org.Visibility.IsLimited}}<span class="ui large basic horizontal label">{{ctx.Locale.Tr "org.settings.visibility.limited_shortname"}}</span>{{end}}
{{if .Org.Visibility.IsPrivate}}<span class="ui large basic horizontal label">{{ctx.Locale.Tr "org.settings.visibility.private_shortname"}}</span>{{end}}
</span>
{{if .EnableFeed}}
<a class="rss-icon gt-mx-3" href="{{.Org.HomeLink}}.rss" data-tooltip-content="{{ctx.Locale.Tr "rss_feed"}}">{{svg "octicon-rss" 24}}</a>
{{end}}
</div>
{{if $.RenderedDescription}}<div class="render-content markup">{{$.RenderedDescription|Str2html}}</div>{{end}}
<div class="text grey meta">
Expand All @@ -23,6 +20,11 @@
</div>
</div>
<div class="right menu">
{{if .EnableFeed}}
<button class="link-action ui basic label button gt-mr-0" data-tooltip-content="{{ctx.Locale.Tr "rss_feed"}}" data-url="{{$.Org.HomeLink}}.rss">
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry but what's this? What's the purpose of link-action + data-url for the RSS link?

It breaks the link.

{{svg "octicon-rss" 24}}
</button>
{{end}}
<button class="link-action ui basic button gt-mr-0" data-url="{{.Org.HomeLink}}?action={{if $.IsFollowing}}unfollow{{else}}follow{{end}}">
{{if $.IsFollowing}}
{{ctx.Locale.Tr "user.unfollow"}}
Expand Down
9 changes: 6 additions & 3 deletions templates/repo/header.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@
<span class="ui basic label">{{ctx.Locale.Tr "repo.desc.template"}}</span>
{{end}}
</div>
{{if $.EnableFeed}}
<a class="rss-icon gt-ml-3" href="{{$.RepoLink}}.rss" data-tooltip-content="{{ctx.Locale.Tr "rss_feed"}}">{{svg "octicon-rss" 18}}</a>
{{end}}
</div>
{{if $.PullMirror}}
<div class="fork-flag">{{ctx.Locale.Tr "repo.mirror_from"}} <a target="_blank" rel="noopener noreferrer" href="{{$.PullMirror.RemoteAddress}}">{{$.PullMirror.RemoteAddress}}</a></div>
Expand Down Expand Up @@ -55,6 +52,12 @@
</div>
</form>
{{end}}
{{if $.EnableFeed}}
{{/* An extra div-element is not necessary here, as this button does not secretly contain two buttons. */}}
<button class="ui compact small basic button" data-url="{{$.RepoLink}}.rss" data-tooltip-content="{{ctx.Locale.Tr "rss_feed"}}">
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, has this code been tested?

I do not think data-url would work for a RSS link.

Copy link
Contributor Author

@n0toose n0toose Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Gosh, I got so preoccupied with trying to make the icons look consistent enough UI-wise to the point where I may have overlooked the possibility that such an attempt could possibly break the URL.

And here I got super happy that I managed to get a Gitea change in without a change requested or breaking things - I understand that this isn't the first time... I will spend the next few minutes looking into this, issuing a fix and if I don't manage it, I'll send in a Revert and then start over.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could try to use <a class="...." href="{{the RSS link}}">...</a>.

<button> means it requires extra JS code to work with a link or it just submits a form.

link-action means it is an AJAX POST request by data-url.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to take more of your time, so I'll inform you of the following: The faulty behavior seems to come from a botched very-early-in-the-morning rebase. I'll need like 30 minutes to properly fix this and then send a subsequent review that fixes this.

Copy link
Contributor Author

@n0toose n0toose Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think you could try to use ....

Yep, that was my plan. I just made a commit that does this exactly (and explicitly defines role=button so that it works under screen readers), but I'll need a few extra minutes to fully test this.

In the future, I'll try to attach short clips after deciding that the commit is PR ready for self-verification reasons and as well as to ease the review process / cause less stress to maintainers.

{{svg "octicon-rss" 16}}
</button>
{{end}}
<form method="post" action="{{$.RepoLink}}/action/{{if $.IsWatchingRepo}}un{{end}}watch?redirect_to={{$.Link}}">
{{$.CsrfTokenHtml}}
<div class="ui labeled button" {{if not $.IsSigned}}data-tooltip-content="{{ctx.Locale.Tr "repo.watch_guest_user"}}"{{end}}>
Expand Down
8 changes: 5 additions & 3 deletions templates/repo/release_tag_header.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
<a class="{{if .PageIsTagList}}active {{end}}item" href="{{.RepoLink}}/tags">{{ctx.Locale.PrettyNumber .NumTags}} {{ctx.Locale.TrN .NumTags "repo.tag" "repo.tags"}}</a>
{{end}}
</h2>
{{if .EnableFeed}}
<a class="rss-icon gt-mx-3" href="{{.RepoLink}}/{{if .PageIsTagList}}tags{{else}}releases{{end}}.rss" data-tooltip-content="{{ctx.Locale.Tr "rss_feed"}}">{{svg "octicon-rss" 18}}</a>
{{end}}
</div>
{{if .EnableFeed}}
<a class="ui small button" href="{{.RepoLink}}/{{if .PageIsTagList}}tags{{else}}releases{{end}}.rss">
{{svg "octicon-rss" 18}} {{ctx.Locale.Tr "rss_feed"}}
</a>
{{end}}
{{if and (not .PageIsTagList) .CanCreateRelease}}
<a class="ui small primary button" href="{{$.RepoLink}}/releases/new">
{{ctx.Locale.Tr "repo.release.new_release"}}
Expand Down
4 changes: 3 additions & 1 deletion templates/repo/view_file.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@
<a download href="{{$.RawFileLink}}"><span class="btn-octicon" data-tooltip-content="{{ctx.Locale.Tr "repo.download_file"}}">{{svg "octicon-download"}}</span></a>
<a id="copy-content" class="btn-octicon {{if not .CanCopyContent}} disabled{{end}}"{{if or .IsImageFile (and .HasSourceRenderedToggle (not .IsDisplayingSource))}} data-link="{{$.RawFileLink}}"{{end}} data-tooltip-content="{{if .CanCopyContent}}{{ctx.Locale.Tr "copy_content"}}{{else}}{{ctx.Locale.Tr "copy_type_unsupported"}}{{end}}">{{svg "octicon-copy" 14}}</a>
{{if .EnableFeed}}
<a class="btn-octicon" href="{{$.FeedURL}}/rss/{{$.BranchNameSubURL}}/{{PathEscapeSegments .TreePath}}">{{svg "octicon-rss" 14}}</a>
<a class="btn-octicon" href="{{$.FeedURL}}/rss/{{$.BranchNameSubURL}}/{{PathEscapeSegments .TreePath}}" data-tooltip-content="{{ctx.Locale.Tr "rss_feed"}}">
{{svg "octicon-rss" 14}}
</a>
{{end}}
{{if .Repository.CanEnableEditor}}
{{if .CanEditFile}}
Expand Down