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.

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

Add alt text to logo #19892

Merged
merged 2 commits into from
Jun 5, 2022
Merged

Add alt text to logo #19892

merged 2 commits into from
Jun 5, 2022

Conversation

Ryuno-Ki
Copy link
Contributor

@Ryuno-Ki Ryuno-Ki commented Jun 4, 2022

Fixes issue #19891

The recommended way
is to use the name of the organisation followed by " logo".
However, since this is my first contribution, I am not entirely sure,
whether this is the best approach here.

The organisation is different from the organisation you can create as
part of the application. Instead, it is more related to the site
hosting the instance. Plus, I don't know how to best handle it when
the logo image is swapped out. Therefore, I use plain "Logo" and hope
that the person visiting the site has enough context.

According to CONTRIBUTING.md
the only translation file to edit as part of the code base is en-US.ini.
I noticed several sections there, but I assume, base stuff go into the top.
A lack of comments made me unsure, whether I guessed correctly.

This PR adds a new translation key (logo) and therefore I added a new
line to the ini file.

I ran make fmt and make lint locally and found no errors or changes.
Do we have tests for the markup? Where would I need to update one?
I only saw tests for .go files (with a few for js). Rebuilding Gitea made the
alt attribute appear with the English text in my local instance.

Signed-off-by: André Jaenisch [email protected]

The recommended way is to use the name of the organisation followed
by "logo". however, since this is my first contribution, I am not
entirely sure, whether this is the best approach here.

The organisation is different from the organisation you can create as
part of the application. Instead, it is more related to the site
hosting the instance. Plus, I don't know how to best handle it when
the logo image is swapped out. Therefore, I use plain "Logo" and hope
that the person visiting the site has enough context.

Signed-off-by: André Jaenisch <[email protected]>
Copy link
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

I think this looks good. Since the logo can be customized per-instance, I think "logo" makes generic sense.
Thanks!

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 4, 2022
@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 5, 2022
@Gusted Gusted added this to the 1.17.0 milestone Jun 5, 2022
@Gusted Gusted added the type/enhancement An improvement of existing functionality label Jun 5, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@89a8b3e). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main   #19892   +/-   ##
=======================================
  Coverage        ?   47.32%           
=======================================
  Files           ?      959           
  Lines           ?   133652           
  Branches        ?        0           
=======================================
  Hits            ?    63245           
  Misses          ?    62707           
  Partials        ?     7700           

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 89a8b3e...eaf3413. Read the comment docs.

@lunny lunny merged commit 73382d2 into go-gitea:main Jun 5, 2022
@Ryuno-Ki Ryuno-Ki deleted the fix-19891-logo-alt-text branch June 5, 2022 15:48
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 5, 2022
* giteaofficial/main:
  Add alt text to logo (go-gitea#19892)
  Limit max-height of CodeMirror editors for issue comment and wiki (go-gitea#18271)
  Implement http signatures support for the API (go-gitea#17565)
  Increment tests time out from 40m to 50m because sometimes the machine is slow (go-gitea#19887)
  fix(CI/CD): correct CI variable. (go-gitea#19886)
  Fix typo (go-gitea#19889)
  Fixing wrong paging when filtering on the issue dashboard (go-gitea#19801)
  Move `/info` outside authorization (go-gitea#19888)
  Fix order by parameter (go-gitea#19849)
  Exclude Archived repos from Dashboard Milestones (go-gitea#19882)
  use exact search instead of fuzzy search for branch filter dropdown (go-gitea#19885)
@wxiaoguang wxiaoguang mentioned this pull request Jun 13, 2022
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
The recommended way is to use the name of the organisation followed
by "logo". however, since this is my first contribution, I am not
entirely sure, whether this is the best approach here.

The organisation is different from the organisation you can create as
part of the application. Instead, it is more related to the site
hosting the instance. Plus, I don't know how to best handle it when
the logo image is swapped out. Therefore, I use plain "Logo" and hope
that the person visiting the site has enough context.

Signed-off-by: André Jaenisch <[email protected]>

Co-authored-by: Lunny Xiao <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

7 participants