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 improvements #99

Closed
wants to merge 2 commits into from
Closed

Accessibility improvements #99

wants to merge 2 commits into from

Conversation

2aces
Copy link

@2aces 2aces commented Apr 5, 2016

Added

  • Accessibility improvement: use of roles and other wai-aria attributes
  • Accessibility improvement: focus management when baguetteBox popup is
    open

References and acknowledgments in code

Closes #98

#Added
- Accessibility improvement: use of roles and other wai-aria attributes
- Accessibility improvement: focus management when baguetteBox popup is
open

References and acknowledgments in code
&:focus {
background-color: rgba(50,50,50,.9);
}

&:hover {
Copy link
Owner

Choose a reason for hiding this comment

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

This could be placed in one statement like so:

&:focus,
&:hover {
   ...
}

@feimosi
Copy link
Owner

feimosi commented Apr 8, 2016

Thanks for your contribution! Looks great, now I need some time to review it.
Just could you remove the version bump and build part? I want to take care of releases myself :)

overlayStatus = true;
lastFocus = document.activeElement;
// change focus to next button if more than one image. Otherwise, focus on close button
if ( 1 < imagesElements.length ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the spaces inside the brackets following the current style?

Copy link
Contributor

Choose a reason for hiding this comment

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

And move the check to the right.

accessibility improvements revised, following notes by feimosi
@2aces
Copy link
Author

2aces commented Apr 8, 2016

Not sure i understood properly all notes, but... anyway, there's a new commit following the suggestions and requests. Hope this works now.

PS: This is my first public contribution and I am using Github desktop. Forgive any mistakes made by this noob. :-)

@XhmikosR
Copy link
Contributor

XhmikosR commented Apr 8, 2016

The branch seems messed up to be honest. Just check the diff.

@XhmikosR
Copy link
Contributor

@feimosi: you want me to cherry pick this and fix it?

@feimosi
Copy link
Owner

feimosi commented Apr 10, 2016

I think we should wait for @2aces to clean this up.

@XhmikosR
Copy link
Contributor

I doubt he can, that is why I offered ;)

@feimosi feimosi changed the title 1.6.0 - accessibility improvements Accessibility improvements Apr 10, 2016
@feimosi
Copy link
Owner

feimosi commented Apr 10, 2016

Ok, let's wait a day or two, then feel free to do it yourself :)

@2aces
Copy link
Author

2aces commented Apr 11, 2016

@feimosi sorry for the mistakes. In order do avoid further mistakes, what would you advise me to do with it?

  • revert to the commit before I made any changes AND then apply my changes following your style and notes or
  • clean up my last commit
  • leave be and don't mess anything else?

@XhmikosR
Copy link
Contributor

@2aces:

git fetch upstream
git rebase -i upstream/master
# properly rebase your branch

@2aces
Copy link
Author

2aces commented Apr 27, 2016

Sorry, @feimosi @XhmikosR I am facing a serious health problem on my shoulder and could not address the issues on my code and my bad commit. It will take a few weeks of therapy for me getting back to code. Therefore:

1 - I welcome anyone cherry-picking it
2 - I had a few more ideas. What is the best course of action, discuss it here in the few moments i feel able to type or code it (properly) when my shoulder is healed?

@XhmikosR
Copy link
Contributor

All right, I'll try to cherry pick this, clean it up and make a new PR.

@feimosi
Copy link
Owner

feimosi commented Apr 30, 2016

@2aces sure, I understand and no problem. Get well soon!

@feimosi feimosi closed this Apr 30, 2016
@feimosi feimosi mentioned this pull request May 27, 2016
@feimosi
Copy link
Owner

feimosi commented May 27, 2016

@2aces I'll merge the current state of the accessibility improvements today (see #112). When you'll be able to discuss the other ideas, please create a new issue.

@2aces 2aces deleted the acessibility-improvements branch August 17, 2016 19:55
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.

None yet

3 participants