-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Improve RSS feed icons #28368
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
Improve RSS feed icons #28368
Conversation
- The RSS Feed icons were placed in a proper button, so that it does not look "inconsistent". This also makes the problem of the button being improperly aligned go away. - The icon that shows on user profiles has not been modified because of a lack of better implementation ideas. - Where applicable, the RSS Feed icon was put directly next to the Follow button (right menu), as both functionalities effectively share the same purpose. - Despite the attempt at achieving less inconsistency, a conscious decision to not add any text to those buttons was made, opting for tooltips instead. "Make it present, but not too annoying." - A special exception was made for the Releases pages (which contains text, not a tooltip), where an RSS feed would be particularly beneficial to users. The fact that the RSS functionality is explicitly optional was taken into account, and these improvements were made with public-facing instances (where the feature works best) in mind.
@@ -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"> |
There was a problem hiding this comment.
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.
@@ -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"}}"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
not look "inconsistent". This also makes the problem of the button
being improperly aligned go away.
of a lack of better implementation ideas.
Follow button (right menu), as both functionalities effectively
share the same purpose.
decision to not add any text to those buttons was made, opting for
tooltips instead. "Make it present, but not too annoying."
text, not a tooltip), where an RSS feed would be particularly
beneficial to users.
The fact that the RSS functionality is explicitly optional was taken
into account, and these improvements were made with public-facing
instances (where the feature works best) in mind.