Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix: escape behavior of popup and dropdown #1183

Merged
merged 6 commits into from
Apr 8, 2019

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Apr 7, 2019

Fixes #1079 .

The issue was caused by the fact that Esc key event was propagated further in the components tree, even being processed by Popup/Dropdown. Necessary instructions to prevent propagation were added to the code.

@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #1183 into master will decrease coverage by <.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1183      +/-   ##
==========================================
- Coverage   82.46%   82.45%   -0.01%     
==========================================
  Files         735      735              
  Lines        8723     8726       +3     
  Branches     1169     1169              
==========================================
+ Hits         7193     7195       +2     
- Misses       1514     1515       +1     
  Partials       16       16
Impacted Files Coverage Δ
...ackages/react/src/components/Dropdown/Dropdown.tsx 61.12% <0%> (-0.17%) ⬇️
packages/react/src/components/Popup/Popup.tsx 75.8% <75%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3905d6f...ddf2bae. Read the comment docs.

@silviuaavram
Copy link
Collaborator

silviuaavram commented Apr 8, 2019

I tested in the docsite with selection and search PRs. Esc looks fixed, although I am not sure why the popup closes when you click on the search input text. or when you Tab into it and then hit Enter.

This happens in this PR, I tried to repro in that sandbox from the issue and it was ok there.

Used the code below in the Inline example in the Docsite.

import React from 'react'
import { Button, Dropdown, Popup } from '@stardust-ui/react'

const inputItems = [
  'Bruce Wayne',
  'Natasha Romanoff',
  'Steven Strange',
  'Alfred Pennyworth',
  `Scarlett O'Hara`,
  'Imperator Furiosa',
  'Bruce Banner',
  'Peter Parker',
  'Selina Kyle',
]

const PopupExampleInline = () => (
  <Popup
    trigger={<Button icon="expand" />}
    content={
      <>
      <Dropdown
        items={inputItems}
        placeholder="Start typing a name"
        noResultsMessage="We couldn't find any matches."
        getA11ySelectionMessage={{
          onAdd: item => `${item} has been selected.`,
        }}
      />
      <Dropdown
        search
        items={inputItems}
        placeholder="Start typing a name"
        noResultsMessage="We couldn't find any matches."
        getA11ySelectionMessage={{
          onAdd: item => `${item} has been selected.`,
        }}
      />
      </>
    }
    inline
  />
)

export default PopupExampleInline

this.close(e, () => _.invoke(this.triggerFocusableDomElement, 'focus'))
e.stopPropagation()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it needed to stop propagation in the popup? As far as understood, the problem was caused in Dropdown which was propagating keyboard event to Popup. Seems like Dropdown changes should fix it, isn't it?

Copy link
Contributor

@sophieH29 sophieH29 Apr 8, 2019

Choose a reason for hiding this comment

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

I believe Dropdown in Popup will be usually used with focus trap behavior. This PR #1180 stop propagation for any keyboard events within FocusTrapZone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sophieH29 why is it needed to stop propagation in the popup? As far as understood, the problem was caused in Dropdown which was propagating keyboard event to Popup

Actually, this makes just one aspect of the problem, the one that is replicated with the codesandbox example in the issue description: #1079 . If it would be the only problem, than yes, changes to the Dropdown would be enough.

However, as it is mentioned in the issue, the same problem applies to Popup -> Popup case, and this behavior is not correct (from issue's description):

The same result will be for multiple Popups.


@sophieH29 I believe Dropdown in Popup will be usually used with focus trap behavior. This PR #1180 stop propagation for any keyboard events within FocusTrapZone

Yes, it should - however, in the meanwhile this fix is necessary

@kuzhelov kuzhelov merged commit a5d017b into master Apr 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/escape-behavior-of-popup-and-dropdown branch April 8, 2019 16:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants