-
Notifications
You must be signed in to change notification settings - Fork 361
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
fix: Spin Button Date tests and sync spec to current behaviour #1640
Conversation
Regression test coverage:Examples without any regression tests:
Examples missing some regression tests:
Example pages with Keyboard or Attribute table rows that do not have data-test-ids:
SUMMARY:55 example pages found. ERROR - missing tests: Please write missing tests for this report to pass. |
b4dc90e
to
bea0560
Compare
Thank you @nschonni |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks so much for the fixes to downarrow! But I don't think we should fix the failing tests related to page down and page up -- those tests are failing because the example code is incorrect, as is tracked in issue #1426. We don't want to have tests that pass because they don't run into the bug that currently exists in the example.
Can you revert the changes to page up/page down in this PR?
0111b6e
to
b9d5234
Compare
@spectranaut I've updated the spec and test to reflect the current behaviour, then #1426 can be used to update it to the new behaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as Matt is ok with the change in the description of the page up
/page down
on the spin button, then I approve this PR.
@spectranaut feel free to cherry pick part of the fix into #1639 if you think this might be slower to land and want to get the test green quicker 😄 |
I cherry picked the first commit ( So if you could drop it from this PR we can consider the rest of the changes :) |
👍 when that one lands I'll rebase and it'll drop out. If i remove it now, the tests will fail 😄 . Although because of the GitHub Actions issues earlier today, there are a bunch of failures right now |
b9d5234
to
7ed964f
Compare
Based on above comments and commits in #1639, I merged that one. Do we still need something from this PR? |
7ed964f
to
bbd2ee7
Compare
@mcking65 the other PR had just the first commit from this PR, now gone because I rebased. The rest of the commits took a similar approach to enable the other tests marked failing and updated the example description to match the current behaviour. The flip side to these changes is that @spectranaut opened the issue #1426 because the current behaviour might not be what is wanted in the future. |
It would be nice to get some eyes on this change -- what is the preferred way for the spin button date picker to behave at month and year boundaries with "page up" and "page down"? Should it always move the spinner 5 up or 5 down? Or, once you get to within 5 of the last day or first day of the month, should it just jump the last or first day of the month? What do you think, @carmacleod, @jongund ? |
Opened #1653 as an alternate approach to change the rollover behaviour |
@spectranaut @carmacleod |
bbd2ee7
to
b83013f
Compare
@jongund commented:
If we are going to change the behavior, let's make that a different issue and PR. Personally, in date pickers, I kind of like the ones where increasing the value just rolls time forward across boundaries, e.g., increasing day on Jan 31 changes date to Feb 1. However, stopping at boundaries is just as valid. Jon, if you think we should have a discussion of the behavior, please raise an issue. I'm going to go ahead with this PR so we at least have documentation and testing that matches the current behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @nschonni!
I'd like to merge, but have some editorial suggestions to simplify the language.
Co-authored-by: Matt King <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, ready to ship now!
#1426 suggests that the behaviour should be updated in the future.
This enables the tests reliably and adds the current undocumented behaviour in the example around some keystrokes.