-
Notifications
You must be signed in to change notification settings - Fork 3
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
io-1156: Add combobox component #491
base: main
Are you sure you want to change the base?
Conversation
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.
Overall very good! This is not a complete review, but gives you something to work on while I look over the rest :)
<div class="ods-devtools-code__box__code"> | ||
<div class="ods-combobox"> | ||
<div class="ods-combobox__select"> | ||
<input class="ods-combobox__input ods-combobox__input--focus" id="ods-combobox-1" type="text" autocomplete="off" aria-label="Label text" placeholder="Select an item from the list" /> |
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.
why focus modifier here? this is not a class that should be used other than for test purposes?
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.
When the dropdown is expanded, the input has the focus, therefore it must show the focus style. If I don't add this class, a small gap will be visible between the input and the dropdown and that scenario of an input without focus and dropdown open doesn't happen.
Maybe we should only show the VUE component and developer tab the rest of uses.
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.
Either that or show it without dropdown / focus, it's not good that they copy debug classes for code examples me thinks
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 dropdown without dropdown will be weird XD.
Agree with that, better not to add a test class there.
I guess the best is to replace them with vue components, so both dropdown expands/collapse and show the proper style and content on focus/not focus
left: -0.25rem; | ||
right: -0.25rem; | ||
background-color: colors.$white; | ||
box-shadow: 0 7px 10px 0 rgb(0 0 0 / 10%); |
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.
rem?
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.
Thinking it is a bit of fuss to use box-shadow: 0 0.438rem 0.625rem 0 rgb(0 0 0 / 10%);
for something that is ok to have the same size in all breakpoints.... same thing happens with 1px border.
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.
But then shadow and border will not scale the rest if you increase font size in browser, how will that look? Can you do a test?
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 are going with rem values because that is what is the foundation of designsystem, scalable values
@extend %ods-padding-4; | ||
@extend %ods-text--weight-medium; | ||
|
||
border-top: 1px solid colors.$gray-light; |
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.
rem?
'placeholder': 'Type \'i\' to search or up and down to navigate', | ||
}" | ||
:id="id" | ||
:input-id="inputId" |
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 doesn't need to be a prop, it can simply build on id to be unique, "id + 'input'" and then the consumer doesn't have to specify it
}" | ||
:id="id" | ||
:input-id="inputId" | ||
:listbox-id="listboxId" |
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 doesn't need to be a prop, it can simply build on id to be unique, "id + 'listbox'" and then the consumer doesn't have to specify it
@@ -0,0 +1,233 @@ | |||
<!-- Overview mode --> |
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.
Not possible to click on arrow to close combobox dropdown
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.
If you use backspace in the example after typing "item 1" and remove 1 and space the dropdown stays open, but if you do the same typing "item 2" and using backspace, no dropdown
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.
If you have an items array like this:
{
text: 'abc'
},
{
text: 'bcd'
},
{
text: 'cde'
}
it is impossible to type bcd or cde, only abc works
No description provided.