-
Notifications
You must be signed in to change notification settings - Fork 0
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
bugfix/3385_filter_account_popper_visibility #76
Conversation
looks good! thank you. Can we just implement a null check on the query selector when focussing the first input?
|
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're losing functionality it seems?
this._tableWrpHeight ||= tableWrap?.style.minHeight | ||
tableWrap.style.minHeight = `${popperHeight}px` | ||
} | ||
const firstInputEle = this.popperInstance.state.elements.popper.querySelector('form input:not([class=hidden])') |
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.
Use full names for variables (firstInputElement).
Should we also look for selects/textarea?
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 approved the changes. we only now have a bug for the tables that are on the lower end of the page.. (e.g. last table on the sales order overview)
textarea indeed, i dont know if we use it to filter on anything though. we just put focus on it so we can start typing a query without having to actually move into the dropdown/textfield to start typing the query. it saves one click.
Do we use selects?
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.
nope selected value will be prepopulated in input area
https://github.com/boxture/oms/issues/3385