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

accessibility: add focus state styles to interactive elements #1153

Merged
merged 1 commit into from
Apr 26, 2020
Merged

accessibility: add focus state styles to interactive elements #1153

merged 1 commit into from
Apr 26, 2020

Conversation

nisarhassan12
Copy link
Contributor

This Pr addresses the following from #1149

image

This is how it looks with the changes in place.

image

The dotted outline does not look pretty and provides no benefits to users that are using a mouse. So I have made sure that:

  • it only appears when the user is navigating via a keyboard.
  • and if in the middle of Navigation let say the user swaps from using a keyboard to a mouse or vice versa for navigation then the handlers will be injected or ejected accordingly.

@shcheklein
Copy link
Member

@nisarhassan12 CI failed, please check when you have time

@shcheklein shcheklein requested a review from jorgeorpinel April 15, 2020 22:11
@shcheklein
Copy link
Member

@nisarhassan12 can we highlight buttons with some "glow" effect around them? border might look find around links (sometimes), but buttons should have some nice effect. Check the Slack website that you shared - they have some nice effects for buttons.

@shcheklein shcheklein temporarily deployed to dvc-landing-keyboard-na-6yutbw April 16, 2020 01:27 Inactive
@nisarhassan12
Copy link
Contributor Author

Thanks! @shcheklein hopefully I will try to fix the CI and adding the glow effect to the buttons tomorrow.

jorgeorpinel
jorgeorpinel previously approved these changes Apr 16, 2020
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

LGTM. I see buttons already have a special style for this too:
image
image

@jorgeorpinel jorgeorpinel dismissed their stale review April 16, 2020 22:26

No wait I was wrong...

@jorgeorpinel
Copy link
Contributor

Other buttons are missing that indeed
image
image

@jorgeorpinel
Copy link
Contributor

Also, and not withing the scope of this PR, but navigating with tab seems to be using hidden elements (probably responsive stuff?) because from [Get Started] to [Download] there's like 15 jumps.

@nisarhassan12
Copy link
Contributor Author

nisarhassan12 commented Apr 17, 2020

Also, and not withing the scope of this PR, but navigating with tab seems to be using hidden elements (probably responsive stuff?) because from [Get Started] to [Download] there's like 15 jumps.

Thanks, @jorgeorpinel. yeah, there is the hamburger menu and the links( this is mentioned in #1153).

image

I will make a separate PR for that.

@shcheklein shcheklein temporarily deployed to dvc-landing-keyboard-na-6yutbw April 17, 2020 01:06 Inactive
@nisarhassan12
Copy link
Contributor Author

nisarhassan12 commented Apr 17, 2020

About the CI failure maybe it's just me but I think code climate is maybe too strict here depending on what we are trying to do here. indeed the code blocks are similar but they are doing the opposite of each other. @shcheklein do you have any ideas on how one can go about refactoring this. I can split the 2 lines that are responsible for adding/removing event listeners by splitting them into a separate module exported from a different file that both functions can use but that is too much in my opinion i.e they are already function calls with different arguments. What do you say 🙂?

image

@shcheklein
Copy link
Member

@nisarhassan12 yeah, no need to solve all code climate warning! I was talking about CircleCi.

@shcheklein
Copy link
Member

It also does not highlight docs sidebar properly.

@nisarhassan12
Copy link
Contributor Author

nisarhassan12 commented Apr 17, 2020

let's put all this into src/utils/accessibility.ts looks like one file per function is too much

@shcheklein I tried this I was using a similar approach but it would cause Circle CI to fail because of the way the event handlers work, splitting them into different files was kind of a workaround so that Circle CI does not fail:

image

Should I do this anyway even if Circle CI fails?

@shcheklein
Copy link
Member

@nisarhassan12 I see. splitting into two files probably not a good solution - I would explore if there is a better way to do this

@nisarhassan12
Copy link
Contributor Author

nisarhassan12 commented Apr 17, 2020

It also does not highlight docs sidebar properly.

Thanks, I also noticed a few other places where it does not highlight properly I will update the Pr soon and also add some other effect to the buttons other than the outline.

@shcheklein shcheklein temporarily deployed to dvc-landing-keyboard-na-6yutbw April 17, 2020 09:47 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-keyboard-na-6yutbw April 17, 2020 10:44 Inactive
@nisarhassan12
Copy link
Contributor Author

nisarhassan12 commented Apr 17, 2020

Thanks, I also noticed a few other places where it does not highlight properly I will update the Pr soon and also add some other effect to the buttons other than the outline.

I have made the changes. @shcheklein please have a look whenever you'll have time.

@jorgeorpinel
Copy link
Contributor

This particular button doesn't feel focused going from transparent to white backgroud:

image
image

The other ones in the landing page at least look good but the dashed border is gone from all buttons. Should we keep both things?

@shcheklein
Copy link
Member

I asked to remove dashed board from buttons, it think it looks nice that button just have different glow/highlight - looks like enough visually.

@shcheklein shcheklein merged commit 997d9c2 into iterative:master Apr 26, 2020
@shcheklein
Copy link
Member

thanks @nisarhassan12 ! cool stuff.

@nisarhassan12 nisarhassan12 deleted the keyboard-navigation branch April 26, 2020 09:21
iksnagreb pushed a commit to iksnagreb/dvc.org that referenced this pull request Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants