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

Proto/multi dropdown #334

Closed
wants to merge 24 commits into from
Closed

Proto/multi dropdown #334

wants to merge 24 commits into from

Conversation

silviuaavram
Copy link
Collaborator

@silviuaavram silviuaavram commented Oct 8, 2018

This PR contains the changes from #244 since it became pretty complicated to rebase master changes. Will update on its progress.

Do not merge yet! It still contains a work-around for Input in order to pass role="presentation" to its <div> so the other Input examples are at the moment broken.

  • Conformance test
  • Minimal doc site example
  • Stardust base theme
  • Teams Light theme
  • Teams Dark theme
  • Teams Contrast theme
  • Confirm RTL usage
  • W3 accessibility check
  • Stardust accessibility check
  • Update glossary props table
  • Update the CHANGELOG.md

</li>
<li>Open/Close dropdown on click and keyboard Enter/Up/Down/Esc.</li>
<li>Provide a callback for handling the selection of an element from the list.</li>
<li>Append aria roles and other attributes to make it accessible with screen reader.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: aria -> ARIA

const inputClasses = classes.input

return (
<ElementType className={classes.root} onChange={onChange} {...rest}>
Copy link
Contributor

Choose a reason for hiding this comment

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

note that for onChange event here we are not providing the callback's signature that is advertised in prop types (namely, (e, props) => ...): here we are not providing any component's props to the callback

ref: this.handleInputRef,
},
})}
{this.computeIcon() &&
Copy link
Contributor

@kuzhelov kuzhelov Oct 14, 2018

Choose a reason for hiding this comment

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

note, renderIcon support is lost with suggested changes. The same applies to renderInput

Copy link
Contributor

Choose a reason for hiding this comment

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

We can iterate again on the Input once the new PR for it is merged, and can see if there is any other issues. This are just temporary fixes anyway.

Copy link
Contributor

@kuzhelov kuzhelov left a comment

Choose a reason for hiding this comment

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

very good prototype, lets address comments before merging

@kuzhelov kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Oct 14, 2018
@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #334 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   89.94%   89.95%   +<.01%     
==========================================
  Files          64       64              
  Lines        1253     1254       +1     
  Branches      162      162              
==========================================
+ Hits         1127     1128       +1     
  Misses        123      123              
  Partials        3        3
Impacted Files Coverage Δ
src/components/Input/Input.tsx 100% <100%> (ø) ⬆️

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 3d7cb18...5d09837. Read the comment docs.

@silviuaavram silviuaavram added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Oct 22, 2018
@levithomason
Copy link
Member

@silviuavram Note, if you slightly adjust the PR description then this PR will automatically close the associated issue once it is merged. See here:

https://help.github.com/articles/closing-issues-using-keywords

Associated issue is #388

{this.state.message}
</span>
{this.renderSelected()}
<Input
Copy link
Contributor

Choose a reason for hiding this comment

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

this Input element can be wrapped by Ref that is now exported from Stardust - no need to change Input component's API in that case

@silviuaavram
Copy link
Collaborator Author

This PR preceded the Dropdown component, by having this prototype as reference. closing as the Dropdown component has been implemented.

@silviuaavram silviuaavram deleted the proto/multi-dropdown branch December 10, 2018 10:05
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.

4 participants