Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

Some Wave/WebAim recommended fixes. #1635

Merged
merged 1 commit into from
Nov 20, 2020
Merged

Conversation

fairlighteth
Copy link
Contributor

No description provided.

@gnosis-info
Copy link

Travis automatic deployment:

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

it helps with the review if you add some description and/or screenshot if the change is visual

justify-content: center;
cursor: pointer;
outline: 0;

/* TODO: Provide alternative :focus styling because of using outline: 0; */
Copy link
Contributor

Choose a reason for hiding this comment

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

is this sth u intended to do in this PR and forgot, or sth we need to address in the future?
not sure if I understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intended to be addressed in the future. As this would apply to a lot more items.

Copy link
Contributor

@anxolin anxolin Nov 20, 2020

Choose a reason for hiding this comment

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

Okay. Maybe here is not worth it. For bigger things, or things we don't want to forget, we add a task as an issue or in the backlog so we don't forget. Although now we are not attending so much issues of this gpv1 (so we might even overlook)

Copy link
Contributor

@Velenir Velenir left a comment

Choose a reason for hiding this comment

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

Where are these fixes recommended? Did you run code through some validator?
If this is because of lat & aria attributes, I'm pretty sure we don't have them in a thousand places
If you want them in, that would be one big PR...

@fairlighteth
Copy link
Contributor Author

Where are these fixes recommended? Did you run code through some validator?
If this is because of lat & aria attributes, I'm pretty sure we don't have them in a thousand places
If you want them in, that would be one big PR...

Have been using https://chrome.google.com/webstore/detail/wave-evaluation-tool/jbbplnpkjmmeebjpijfedlgcdilocofh which is the WAVE evaluation tool.

My approach is to just tackle things step by step, instead of doing one big PR. These are recommended changes only for improving accessibility.

@W3stside
Copy link
Contributor

While I think this is good, I think we should postpone this for until we have fully implemented playwright integratino so that it can test any potential regressions from adding these.

Not saying I think these will break anything (they shouldnt) BUT if we start changing tons of files and we mess sth up that isn't caught from linter or else, we'll be in trouble

@fairlighteth
Copy link
Contributor Author

fairlighteth commented Nov 20, 2020

While I think this is good, I think we should postpone this for until we have fully implemented playwright integratino so that it can test any potential regressions from adding these.

Not saying I think these will break anything (they shouldnt) BUT if we start changing tons of files and we mess sth up that isn't caught from linter or else, we'll be in trouble

Fair point. As you pointed out, adding tags for accessibility should be validated HTML by default. But in practice things can happen.

@fairlighteth fairlighteth merged commit 43ca2ef into develop Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants