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

Add date picker spin button example for issue 125 #1053

Merged
merged 52 commits into from
Jul 13, 2019

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Jun 27, 2019

Adds a spin button example to resolve issue #125. The example demonstrates using spin buttons to build a date picker.

Preview Link

View WIP on spin button date picker


Preview | Diff

@spectranaut
Copy link
Contributor

@mcking65 @jongund I reviewed this one as well, functionality and looks are good. I pushed a commit to satisfy the html and css linters. A few minor issues:

  1. Keyboard table "Spin Button (Day, Month and Year)", up/down arrows and home and end

    • Date and month spinners both wrap and this is described in the "Function" column. However, the year spinner does not wrap and this is not mentioned in the "Function" column. Should the year wrap for consistency? Should the fact that the year does not wrap be mentioned?
  2. Attribute table "Spin Button (Day, Month and Year)", row "aria-valuetext="month""

    • The date spinner also has an "aria-valuetext" (set to the ordinal value of the month, for example: "first", "second", "third", etc). This should probably be mentioned in this description along with month (right now only the aria-valuetext for the month spinner is described).
    • The year spinner also has an "aria-valuetext" set and it is identical to "aria-valuenow", which I believe means it should be removed.

@jongund
Copy link
Contributor Author

jongund commented Jul 8, 2019

@spectranaut

Valarie,

  1. The spin button for dates that I modeled the example from only wrapped for day and month, the range for years did not wrap. I don't have a strong opinion on the issue, so if the group wants it to wrap that's fine with me. It does demonstrate that spin button's don't have to wrap.
  2. I will update the documentation on the spin button for date to indicate how aria-valuetext is used.
  3. I will fix the bug for year including aria-valuetext attribute.

@jongund
Copy link
Contributor Author

jongund commented Jul 8, 2019

@spectranaut @mcking65
I have made the changes Valarie suggested and have checked them in. I did not change the behavior of the year spin button to wrap, let me know if it is important to make this change.

@mcking65
Copy link
Contributor

mcking65 commented Jul 9, 2019

I'm conflicted what to say or do about touch screen reader support. This form of spin button implementation cannot work with touch screen readers. It cannot be made to work until we have interactions supported by AOM.

However, between now and the time we can use AOM, it is possible to make a spinbutton touch accessible by moving the increment and decrement buttons outside the spinbutton div and then making them focusable with tabindex -1. The downside is more clutter for desktop screen reader users. The upside is more users have access.

So, should we:

  1. Add a note stating this form of spinbutton implementation is not accessible with touch screen readers and open an issue to address it later.
  2. Change this example.

While we have stated in the Read Me First section that touch screen reader support is not yet addressed by the APG, we have also been working at avoiding increasing touch debt. This seems like a good example of a place where we might want to avoid increasing debt.

@mcking65 mcking65 changed the title Issue125 spinbutton datepicker example Add date picker spin button example for issue 125 Jul 9, 2019
mcking65 and others added 3 commits July 9, 2019 14:25
…example to make the increase and decrease buttons real buttons to support touch devices
…3c/aria-practices into issue125-spinbutton-datepicker-example
@jongund
Copy link
Contributor Author

jongund commented Jul 9, 2019

@spectranaut @mcking65

I have updated the datepicker spin button example to make the increment and decrement buttons real buttons to support touch devices and given them a tabindex=-1. The buttons are also outside the div with the spinbutton role.

I tried to update the test file and made some progress, but didn't have time to finish it, so if Valarie could fix it that would be great.

I can work on any changes Wednesday afternoon, i will be out Wednesday morning.

@jongund
Copy link
Contributor Author

jongund commented Jul 10, 2019

@spectranaut @mcking65

I was able to get a little more time to work on the changes to the test file and got the bugs fixed, so the tests all pass the update date picker spin button example.

@jongund
Copy link
Contributor Author

jongund commented Jul 10, 2019

@spectranaut @mcking65

Updated the documentation and test cases, so ready for further review.

Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

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

Tests look good, just one suggestion that is not necessary right now :)

const valuesMonth = ['January', 'February', 'March', 'April', 'May', 'June', 'July', 'August', 'September', 'October', 'November', 'December'];


var getDaysInMonth = function (year, month) {
Copy link
Contributor

Choose a reason for hiding this comment

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

to get the days in month, you could replace this logic with:

let d = new Date(year, month+1, 0);
return d.getDate();

jongund and others added 27 commits July 12, 2019 07:55
…example to make the increase and decrease buttons real buttons to support touch devices
@mcking65 mcking65 merged commit 8a3fea1 into master Jul 13, 2019
@mcking65 mcking65 deleted the issue125-spinbutton-datepicker-example branch July 13, 2019 04:55
michael-n-cooper pushed a commit that referenced this pull request Jul 13, 2019
Add date picker spin button example for issue 125 (pull #1053)

To resolve issue #125, add an example that uses the spin button pattern to create a date picker.
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.

3 participants