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

Made the logviewer more accessible #8213

Merged
merged 13 commits into from
Aug 31, 2020
Merged

Made the logviewer more accessible #8213

merged 13 commits into from
Aug 31, 2020

Conversation

RachBreeze
Copy link
Contributor

  1. By adding the name of the log viewer to the title of the page
  2. Changing most links with a click action to buttons
  3. Adding aria-described by to help buttons be more informative to the screen reader user, where they have been styled visually.

Note that the "Save Search" isn't localised in the aria describe by, this replicates the popup which is also not localised.

Rachel Breeze and others added 3 commits May 31, 2020 19:36
…er to the title of the page, changing most links with a click action to buttons and adding aria-described by to help buttons be more informative to the screen reader user, where they have been styled visiually.
@nul800sebastiaan
Copy link
Member

@RachBreeze I assume the changes in umbeditorheader.directive.js were not meant for this PR, they seem unrelated?

@nul800sebastiaan
Copy link
Member

Ah, don't mind me, I see what's happening. Is this something that will need to be added to new PRs as well to be able to update the title for anything else you might want to improve? I just want to make sure we don't have to repeat this code over and over again everywhere 😄

Copy link
Member

@nul800sebastiaan nul800sebastiaan left a comment

Choose a reason for hiding this comment

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

Alright, this seems to be working for the most part, but it will require some styling updates, switching to <button> elements has significantly changed the view which use to look something like

image

And now the second column has moved a lot further to the right, also the longer SourceContext message has a weird wrapping with an indentation on the second line that's not supposed to be there:

image

Can you please have a look at that @RachBreeze ?

@RachBreeze
Copy link
Contributor Author

RachBreeze commented Jun 21, 2020

Ah, don't mind me, I see what's happening. Is this something that will need to be added to new PRs as well to be able to update the title for anything else you might want to improve? I just want to make sure we don't have to repeat this code over and over again everywhere 😄

There is still some work to do around titles in the back office, but it's getting fewer and fewer. I'm with you on not repeating code over and over. But every page should have a meaningful title, which is sometime hard for an app, https://www.w3.org/TR/WCAG21/#page-titled (2.4.2)

@RachBreeze
Copy link
Contributor Author

RachBreeze commented Aug 16, 2020

Hi @nul800sebastiaan
Thank you for your feedback
I've updated the styling of the buttons on the properties table and fixed a spacing issue around the entry:
<div ng-if="log.Exception" class="exception">

The source context now wraps:
image

I've taken a look at the spacing issue and it appears on Umbraco Cloud 8.6.4 but it doesn't look to be this commit. The screenshot below shows the 8.6.4 site at the top and my localhost at the bottom.

image

Also the styling of the exception has remained unchanged:

image

@RachBreeze
Copy link
Contributor Author

Further investigations needed following feedback from @poornimanayar , so hold this thought thank you

RachBreeze and others added 3 commits August 22, 2020 16:15
Merge latest Umbraco v8 into my repo
…into v8/tmplogvieer

# Conflicts:
#	src/Umbraco.Web.UI.Client/src/views/logviewer/search.html
@RachBreeze
Copy link
Contributor Author

Hello @nul800sebastiaan
I've updated this to use the latest v8/contrib branch

# Conflicts:
#	src/Umbraco.Web.UI.Client/src/views/logviewer/search.html
Added type="button" on all new buttons
Re-added the rel attribute on all the search links, this is for security reasons. noopener is better that noreferrer
@nul800sebastiaan nul800sebastiaan merged commit 108ea2d into umbraco:v8/contrib Aug 31, 2020
@nul800sebastiaan
Copy link
Member

Thanks again @RachBreeze - this now looked mostly good!
Make sure to check my 2 additional commits:
fd32648
301f73f

Apparently an <a> always needs a href attribute. Make sure to not remove the rel attribute either unless you have a specific reason for that.

Additionally, always add type="button" to <button> elements otherwise they display the behavior of submitting any <form> on the page and lastly all <i> elements should get aria-hidden="true" since screen readers don't read these properly.

I've made these changes for you now so don't worry about that for this PR, all good, merging! 👍

Thanks again, top job!! 🏅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants