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

Address some issues introduced by the a11y PR #477

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Apr 27, 2017

Sorry this took so long! This should address issues raised in #427.

Namely, a handful of things happen in this PR:

  1. The keyboard shortcuts panel button is now transparent instead of white. It also has a higher z-index so it doesn't flash in transitions.
  2. The panel has an overflow of auto so it doesn't totally crap out on single month datepickers
  3. The alignment of text in the panel has been adjusted a bit to support the single month case.
  4. enableOutsideDays was having some weirdness with using the keyboard to transition months. This has been fixed.
  5. There is now a prop to hide the panel entirely. The recommendation is still to either style the panel to best suit your use-case or to communicate the keyboard shortcuts elsewhere, but yaknow.

to: @airbnb/webinfra

lencioni
lencioni previously approved these changes Apr 27, 2017
Copy link
Contributor

@lencioni lencioni left a comment

Choose a reason for hiding this comment

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

Sorry this took so long!

I don't think you need to apologize. You are doing a great job!

@@ -17,10 +17,11 @@
.DayPickerKeyboardShortcuts__show {
width: 22px;
position: absolute;
z-index: 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 2 here? Can this be based off of a variable to make the dependency more explicit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea!

@coveralls
Copy link

coveralls commented Apr 27, 2017

Coverage Status

Coverage decreased (-0.4%) to 83.512% when pulling 6212751 on maja-fix-a11y-issues into 3815516 on master.

@coveralls
Copy link

coveralls commented Apr 27, 2017

Coverage Status

Coverage remained the same at 83.961% when pulling eaf361a on maja-fix-a11y-issues into 3815516 on master.

@sag1v
Copy link
Contributor

sag1v commented Apr 27, 2017

@majapw Great work, thank you.

There is now a prop to hide the panel entirely

What is the name of the prop? (not that i will use it but yaknow lol)

@majapw
Copy link
Collaborator Author

majapw commented Apr 27, 2017

hideKeyboardShortcutsPanel on both the DRP and the SDP

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I'm a very strong -1 for hiding the explanatory shortcut panel when shortcuts still work - I think if they work, the explanation needs to be easily reachable, otherwise people might accidentally type something and have no idea why it triggered a change.

@majapw
Copy link
Collaborator Author

majapw commented Apr 27, 2017

@ljharb while I believe we should provide a reasonable default option for explaining the keyboard shortcuts (which we do, with the panel), I think that because we are providing a tool for developers to integrate into their own products, we should allow for them to customize how they decide to convey this information to their users, whether it is embedded on the page directly, in a modal separate from the datepicker completely, or something I haven't yet considered. We should not force developers to use our solution, and that is why I added the hideKeyboardShortcutsPanel option.

My first recommendation would still be to use css overrides to style the panel to fit your use case, but I want to provide an option to do your own thing as well. This is why I think this prop makes a lot of sense.

@ljharb
Copy link
Member

ljharb commented Apr 27, 2017

I'm still not a fan of hiding the shortcut panel, but after some offline discussion with @majapw I'm definitely not a blocker.

@coveralls
Copy link

coveralls commented Apr 27, 2017

Coverage Status

Coverage remained the same at 83.961% when pulling bf6fa0e on maja-fix-a11y-issues into 3815516 on master.

@coveralls
Copy link

coveralls commented Apr 27, 2017

Coverage Status

Coverage remained the same at 83.961% when pulling e301ced on maja-fix-a11y-issues into 3815516 on master.

@majapw majapw merged commit 5b54d63 into master Apr 27, 2017
@majapw majapw deleted the maja-fix-a11y-issues branch April 27, 2017 23:58
@kbariotis
Copy link

Aw! I came too late! :( Shouldn't README be updated too with the newly added hideKeyboardShortcutsPanel flag?

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.

6 participants