-
Notifications
You must be signed in to change notification settings - Fork 65
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
1565 - Add typeahead dropdown icon/button #1735
1565 - Add typeahead dropdown icon/button #1735
Conversation
This pull request is being automatically deployed with Vercel (learn more). dojo.widgets – ./🔍 Inspect: https://vercel.com/dojo/dojo.widgets/6NGGzjUkWS36h8swiA7Uh2bDYgGN widget-test-docs – ./🔍 Inspect: https://vercel.com/dojo/widget-test-docs/BznWtFVvGuBVNeBGXqF3bTjRaTgM |
Codecov Report
@@ Coverage Diff @@
## master #1735 +/- ##
==========================================
+ Coverage 90.36% 90.43% +0.06%
==========================================
Files 94 94
Lines 5129 5143 +14
Branches 1398 1403 +5
==========================================
+ Hits 4635 4651 +16
+ Misses 243 241 -2
Partials 251 251
Continue to review full report at Codecov.
|
label, | ||
leading, | ||
trailing: showDropDownButton && ( | ||
<button |
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.
We couldn't use the dojo button here?
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.
This is following the same pattern that the Select
widget is using. If we want to change that pattern both should be changed together.
src/typeahead/index.tsx
Outdated
@@ -53,6 +55,8 @@ export interface TypeaheadProperties { | |||
itemDisabled?: ListProperties['disabled']; | |||
/** Flag to indicate if values other than those in the resource can be entered, defaults to true */ | |||
strict?: boolean; | |||
/** Flag to indicate if drop down arrow should be shown in trailing section of text input, defaults to false */ | |||
showDropDownButton?: boolean; |
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.
feels like a bit of a mouthful! I don't have a better idea other than we could potentially drop show
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.
I prefer dijit's hasDownArrow
if that helps.
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.
@matt-gadd That does help! Thanks, I'm sold.
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.
A couple of comments
background: unset; | ||
|
||
appearance: none; | ||
-moz-appearance: none; |
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.
do we need these? doesn't the build generate these for us? cc @matt-gadd
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.
It does not appear to. I just tested removing them and they are not in the end product.
Several other widgets have them in their css too.
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.
One question but otherwise 👍
Type: feature
The following has been addressed in the PR:
.dojorc
theme.variant()
is added to the root domnodetheme.compose
like thisDescription:
Add typeahead dropdown icon/button
Resolves #1565