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.

Sign up for GitHub

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

Jump to bottom

Fix Calendar component PREV/NEXT month, year, and "Go to today" handlers firing twice #4662

Merged
merged 2 commits into from
Apr 25, 2018
Merged

Fix Calendar component PREV/NEXT month, year, and "Go to today" handlers firing twice #4662

merged 2 commits into from
Apr 25, 2018

Conversation

KevinTCoughlin
Copy link
Member

Pull request checklist

Description of changes

In #3741 we refactored all the Calendar <span/> nodes to <button/>, which was a good change 👍 . However, React invokes onClick for KeyboardEvent if the keyCode is SPACE. Since each <button/> node had an onClick and onKeyDown it would invoke the PREV/NEXT month, year, and also "Go to today" callbacks twice.

Solution Options:

I went with # 1, but there's an alternative solution:

  1. If we wish to still allow for ENTER to invoke the <button/> then the change is what is in this PR. We need to only invoke the onKeyDown callbacks if the key was ENTER.
  2. If we only want SPACE to invoke the callback then we can safely remove the onKeyDown handlers.

Since # 2 would would change prior behavior so I left went with option # 1.

Focus areas to test

  • Use the Calendar component's PREV/NEXT month and year buttons with both the mouse and keyboard.
  • The time unit either month or year should change 1 unit per invocation.
  • "Go to today" should also only fire once, but you'll likely need to set a break-point to verify.

Note: that the UI doesn't update when "Go to today" is invoked. Looks like a preexisting bug on master that I'll look into but may file separately. I assume the UI updated at one point, but need to check behavior in prior commits.

@lorejoh12 lorejoh12 merged commit 0ee2136 into microsoft:master Apr 25, 2018
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Apr 26, 2018
@Markionium
Merge branch 'master' into mapol/noGlobalClassNames
4196e29 
* master: (42 commits)
  Applying package updates.
  ProgressIndicator: Finish conversion to mergeStyles (microsoft#4595)
  Fix props validation for Breadcrumb (microsoft#4666)
  No unused vars part of ts (microsoft#4670)
  Picker/Autofill: fixes several minor bugs. (microsoft#4569)
  Fix Calendar component PREV/NEXT month, year, and "Go to today" handlers firing twice (microsoft#4662)
  Applying package updates.
  Merge styles order (microsoft#4664)
  Fabric component: revert class change and make it backwards compatible (microsoft#4671)
  Addressing Issue microsoft#3707 - OverflowSet: Add the ability to set aria-label (microsoft#4667)
  Fix input type for Tile ARIA label prop (microsoft#4668)
  Fix theme slots for DetailsList header colors (microsoft#4658)
  Applying package updates.
  Jolore/calendar updates (microsoft#4643)
  Remove wordWrap setting. (microsoft#4657)
  Pivot: convert to mergeStyles - part 1  (microsoft#4656)
  Use the `data-is-scrollable` attribute on the correct ScrollablePane div (microsoft#4602)
  Applying package updates.
  Remove unused iconClassName prop from Nav.types (microsoft#4634)
  Jest snapshots: classes in animations should autoexpand. (microsoft#4647)
  ...
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request Apr 26, 2018
@Markionium
Merge branch 'master' into mapol/allow-overriding-root-element-on-link
6f05b43 
* master: (34 commits)
  Applying package updates.
  ProgressIndicator: Finish conversion to mergeStyles (microsoft#4595)
  Fix props validation for Breadcrumb (microsoft#4666)
  No unused vars part of ts (microsoft#4670)
  Picker/Autofill: fixes several minor bugs. (microsoft#4569)
  Fix Calendar component PREV/NEXT month, year, and "Go to today" handlers firing twice (microsoft#4662)
  Applying package updates.
  Merge styles order (microsoft#4664)
  Fabric component: revert class change and make it backwards compatible (microsoft#4671)
  Addressing Issue microsoft#3707 - OverflowSet: Add the ability to set aria-label (microsoft#4667)
  Fix input type for Tile ARIA label prop (microsoft#4668)
  Fix theme slots for DetailsList header colors (microsoft#4658)
  Applying package updates.
  Jolore/calendar updates (microsoft#4643)
  Remove wordWrap setting. (microsoft#4657)
  Pivot: convert to mergeStyles - part 1  (microsoft#4656)
  Use the `data-is-scrollable` attribute on the correct ScrollablePane div (microsoft#4602)
  Applying package updates.
  Remove unused iconClassName prop from Nav.types (microsoft#4634)
  Jest snapshots: classes in animations should autoexpand. (microsoft#4647)
  ...
@KevinTCoughlin KevinTCoughlin mentioned this pull request May 4, 2018
Merged
2 tasks
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
@KevinTCoughlin KevinTCoughlin deleted the keco/calendar-keydown-regression branch January 2, 2020 19:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jordandrako jordandrako jordandrako approved these changes

@lorejoh12 lorejoh12 lorejoh12 approved these changes

@mdahamiwal mdahamiwal Awaiting requested review from mdahamiwal

Assignees
No one assigned
Labels
Type: Bug 🐛
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DatePicker: pressing space/Enter on Previous/Next month/year arrow buttons goes 2 months at a time
3 participants
@KevinTCoughlin @jordandrako @lorejoh12