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 alt to the logo, adapt css for logo #35071

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Add alt to the logo, adapt css for logo #35071

merged 1 commit into from
Dec 8, 2022

Conversation

JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Nov 10, 2022 •
edited
Loading

A11y issue regarding alt text for logo on multiple pages

Fix TemplateLayout.php to have an access to $logoUrl outside of the $renderAs === TemplateResponse::RENDER_AS_USER

@JuliaKirschenheuter JuliaKirschenheuter added the 3. to review Waiting for reviews label Nov 10, 2022
@JuliaKirschenheuter JuliaKirschenheuter self-assigned this Nov 10, 2022
@JuliaKirschenheuter
Copy link
Contributor Author

hope i've covered all public / logged in pages
I was not sure about translation: can i leave it like i've done or should i do it different (pleas tell me how)?

@szaimen szaimen requested review from st3iny and Pytal November 10, 2022 10:35
ChristophWurst
ChristophWurst reviewed Nov 11, 2022
View reviewed changes
core/templates/layout.guest.php Outdated Show resolved Hide resolved
core/css/server.css Show resolved Hide resolved
@ChristophWurst
Copy link
Member

/backport to stable25

@JuliaKirschenheuter JuliaKirschenheuter mentioned this pull request Nov 11, 2022
Closed
8 tasks
@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 11, 2022
@ChristophWurst ChristophWurst marked this pull request as draft November 11, 2022 09:44
@ChristophWurst
Copy link
Member

In my humble research I found https://css-tricks.com/replace-the-image-in-an-img-with-css/ and https://stackoverflow.com/questions/17705768/is-it-possible-to-change-img-src-attribute-using-css as possible approaches for the css-overwritten logo in the case of theming.

ChristophWurst
ChristophWurst requested changes Nov 11, 2022
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

The default logo is no longer replaced but simply overlapped

image

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/384-missing_alt_text_for_logo_on_multiple_pages branch from d41d4db to 204bcc3 Compare November 14, 2022 13:59
@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as ready for review November 14, 2022 14:00
@JuliaKirschenheuter JuliaKirschenheuter added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 14, 2022
ChristophWurst
ChristophWurst requested changes Nov 16, 2022
View reviewed changes
core/templates/layout.guest.php Outdated Show resolved Hide resolved
core/templates/layout.public.php Outdated Show resolved Hide resolved
core/templates/layout.user.php Outdated Show resolved Hide resolved
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/384-missing_alt_text_for_logo_on_multiple_pages branch from 204bcc3 to bcf5895 Compare November 22, 2022 15:27
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/384-missing_alt_text_for_logo_on_multiple_pages branch from bcf5895 to 1f75c3f Compare November 23, 2022 14:18
ChristophWurst
ChristophWurst requested changes Nov 23, 2022
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Breaks the logo if there is a custom theme logo

Bildschirmfoto vom 2022-11-23 15-56-03

The image points to /apps/theming/image/logo?useSvg=1&v=47 which returns a 404

@ChristophWurst
Copy link
Member 

Undefined index: logoUrl

/drone/src/core/templates/layout.guest.php:38

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Nov 30, 2022
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/384-missing_alt_text_for_logo_on_multiple_pages branch from 318c772 to 60a3971 Compare November 30, 2022 17:14
@JuliaKirschenheuter
Copy link
Contributor Author

/compile amend /

@nextcloud-command nextcloud-command force-pushed the fix/384-missing_alt_text_for_logo_on_multiple_pages branch from 60a3971 to 08deee5 Compare December 1, 2022 07:34
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/384-missing_alt_text_for_logo_on_multiple_pages branch from 08deee5 to 15bad0c Compare December 1, 2022 12:43
@JuliaKirschenheuter
Copy link
Contributor Author

/compile amend /

@nextcloud-command nextcloud-command force-pushed the fix/384-missing_alt_text_for_logo_on_multiple_pages branch from 15bad0c to ae33719 Compare December 1, 2022 16:52
@JuliaKirschenheuter JuliaKirschenheuter added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Dec 2, 2022
@szaimen
Copy link
Contributor

szaimen commented Dec 6, 2022

Conflicts...

@JuliaKirschenheuter
Add alt to the logo, adapt css for logo
bba5ab0 
Signed-off-by: julia.kirschenheuter <julia.kirschenheuter@nextcloud.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/384-missing_alt_text_for_logo_on_multiple_pages branch from ae33719 to bba5ab0 Compare December 8, 2022 08:28
@JuliaKirschenheuter JuliaKirschenheuter merged commit 11d7cd0 into master Dec 8, 2022
@JuliaKirschenheuter JuliaKirschenheuter deleted the fix/384-missing_alt_text_for_logo_on_multiple_pages branch December 8, 2022 09:36
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Dec 8, 2022
Merged
1 task
@danxuliu
Copy link
Member

This removed the title and, therefore, the name of the shared file in the public share page. Is this known / expected or should I open an issue about it?

Before:
Public-Share-Header-Title-Before

After:
Public-Share-Header-Title-After

@st3iny
Copy link
Member

st3iny commented Jan 17, 2023

This removed the title and, therefore, the name of the shared file in the public share page. Is this known / expected or should I open an issue about it?

Before: Public-Share-Header-Title-Before

After: Public-Share-Header-Title-After

Yes, please open a new issue so that it won't get lost.

@skjnldsv skjnldsv mentioned this pull request Jan 17, 2023
Merged
skjnldsv added a commit that referenced this pull request Jan 17, 2023
@skjnldsv
Fix failed revert for #35071
6144693 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member

skjnldsv commented Jan 17, 2023 •
edited by nickvergessen
Loading

Reverted.

nickvergessen added a commit that referenced this pull request Jan 18, 2023
@nickvergessen
Merge pull request #36194 from nextcloud/revert/35071
634f5a1 
Fix failed revert for #35071
@brunob brunob mentioned this pull request Jan 18, 2023
Closed
9 tasks
@JuliaKirschenheuter JuliaKirschenheuter mentioned this pull request Feb 6, 2023
Closed
9 tasks
@AndyScherzinger AndyScherzinger mentioned this pull request Feb 16, 2023
Closed
11 tasks
@skjnldsv skjnldsv mentioned this pull request Mar 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pytal Pytal Pytal approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

@szaimen szaimen Awaiting requested review from szaimen

@PVince81 PVince81 Awaiting requested review from PVince81

@st3iny st3iny Awaiting requested review from st3iny

Assignees

@JuliaKirschenheuter JuliaKirschenheuter

Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JuliaKirschenheuter @ChristophWurst @szaimen @danxuliu @st3iny @skjnldsv @Pytal