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

fix(dropdown): use Portal to create a11y selection message container #1032

Merged
merged 9 commits into from
Mar 11, 2019

Conversation

silviuaavram
Copy link
Collaborator

@silviuaavram silviuaavram commented Mar 7, 2019

Because the status div was created only when the first selection message was generated, the first time this happened the reader did not announce it.

Now creating the selection status div at component mount, if the Dropdown contains the getA11ySelectionMessage prop.

Using the Popup component to create the message container element.

Updating the examples and added back the add/delete messages to the multiple selection.

@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #1032 into master will decrease coverage by 0.01%.
The diff coverage is 26.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1032      +/-   ##
==========================================
- Coverage   81.64%   81.63%   -0.02%     
==========================================
  Files         675      675              
  Lines        8756     8762       +6     
  Branches     1492     1560      +68     
==========================================
+ Hits         7149     7153       +4     
- Misses       1592     1594       +2     
  Partials       15       15
Impacted Files Coverage Δ
...ackages/react/src/components/Dropdown/Dropdown.tsx 40.1% <26.31%> (+0.42%) ⬆️

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 9fbc73a...5f51a26. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #1032 into master will increase coverage by 0.1%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1032     +/-   ##
=========================================
+ Coverage   81.67%   81.77%   +0.1%     
=========================================
  Files         687      687             
  Lines        8835     8824     -11     
  Branches     1568     1499     -69     
=========================================
  Hits         7216     7216             
+ Misses       1604     1593     -11     
  Partials       15       15
Impacted Files Coverage Δ
...ackages/react/src/components/Dropdown/Dropdown.tsx 40.87% <33.33%> (+1.18%) ⬆️

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 f44caa6...2d39150. Read the comment docs.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

Please refactor this using our Portal component or ReactDOM.createPortal() API, I don't any reasons to use there DOM API directly.

https://reactjs.org/docs/portals.html


render() {
  return (
    <>
      {this.renderDropdown()}
      <Portal open={!!this.state.a11ySelectionStatus}>
        <div role='status' aria-live='polite' style={screenReaderContainerStyles}>
          {this.state.a11ySelectionStatus}
        </div>
      </Portal>
    <>
  )
}

@silviuaavram
Copy link
Collaborator Author

@layershifter last time I wanted to do that it did not work as expected. will give it another go but if it is too complicated I will drop it. the scope of the PR is to fix a bug for embed.

Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

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

👍 looks good, 2 nit

@bmdalex
Copy link
Collaborator

bmdalex commented Mar 8, 2019

@silviuavram @layershifter
depending on the decision to render the a11ySelectionStatus in a Portal or not we should standardize (and maybe even add to dev documentation) this knowledge.

I'm sure other components will need to implement something similar or already do?

@layershifter
Copy link
Member

May be we need to change PR title/changelog entry? 🤔

@layershifter layershifter added ready for merge 🧰 fix Introduces fix for broken behavior. labels Mar 11, 2019
@silviuaavram silviuaavram changed the title fix(dropdown): create selection a11y div at component mount fix(dropdown): use Portal to create a11y selection message container Mar 11, 2019
@silviuaavram silviuaavram merged commit b733cd4 into master Mar 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-getA11ySelectionMessage branch March 11, 2019 17:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants