Skip to content

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

Merged
JuliaKirschenheuter merged 1 commit intomasterfrom
fix/384-missing_alt_text_for_logo_on_multiple_pages
Dec 8, 2022
Merged

Add alt to the logo, adapt css for logo#35071
JuliaKirschenheuter merged 1 commit intomasterfrom
fix/384-missing_alt_text_for_logo_on_multiple_pages

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

JuliaKirschenheuter commented Nov 10, 2022

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 Pytal and st3iny November 10, 2022 10:35
ChristophWurst
ChristophWurst reviewed Nov 11, 2022
View reviewed changes
@ChristophWurst
Copy link
Member

ChristophWurst commented Nov 11, 2022

/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

ChristophWurst commented Nov 11, 2022

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
@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

ChristophWurst commented Nov 30, 2022 

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
ChristophWurst
ChristophWurst reviewed Nov 30, 2022
View reviewed changes
@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

JuliaKirschenheuter commented Dec 1, 2022

/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

JuliaKirschenheuter commented Dec 1, 2022

/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

danxuliu 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

@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

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@Pytal Pytal Pytal 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