-
Notifications
You must be signed in to change notification settings - Fork 12
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
Select: add support for typeahead select #51
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
=======================================
Coverage 99.82% 99.82%
=======================================
Files 43 43
Lines 1677 1706 +29
=======================================
+ Hits 1674 1703 +29
Misses 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2ad416a
to
ce0013f
Compare
@digitronik Hi, can you review please? Also there were some seemingly random failures during the test runs, I had to force push a few times before it all passed, they were tests for other widgets, unrelated to my change though, should we be concerned about that? |
Also this is not urgent, just something I've noticed when working. I wanted to get the items from a select which had a default item selected and when I called |
I've actually found some issues with this. Need to update it. |
This should work now, also I've found out that I need it for the |
I ended up adding a new class for the select as the |
ee8a7c2
to
8d99323
Compare
Ok I think I'm finished with this one, can you review please @digitronik ? :) |
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.
Nice PR 👍
Some suggestions
As we are introducing new widget better to maintain it in README as well.
Special thanks for covering unit tests. I will take a look on more time after changes.
class BaseTypeaheadSelect(BaseSelect): | ||
text = TextInput(locator=".//input") | ||
|
||
def read(self): | ||
return self.browser.get_attribute("value", self.text) |
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.
Lets make this part of widget directly only single method.
Better to use input
as name of child widget.
input = TextInput(locator=".//input")
@@ -19,7 +20,11 @@ class BaseSelect: | |||
https://www.patternfly.org/components/menus/select | |||
""" | |||
|
|||
BUTTON_LOCATOR = ".//button" | |||
BUTTON_LOCATOR = ( |
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.
Better to overwrite in widget itself. Those limitation can break usage. Not tested against console dot so.
@digitronik Is this what you meant? |
I'm not sure why, but the |
@jrusz I'm good with it can you please check jobs |
Thank you. I don't know why the jobs fail, like I said in a comment above they fail seemingly randomly. I don't have permissions to restart them, I'd have to force push, can you try restarting them please? |
BUTTON_LOCATOR = ( | ||
".//button[(contains(@class, '-c-select__toggle') " | ||
"or contains(@class, '-c-menu-toggle')) " | ||
"and not(contains(@class, '-c-select__toggle-clear'))]" |
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.
Consider creating a separate locator for the clear button, and a function to click it.
When there is an item selected it would click on the clear button instead of the toggle button when trying to access items or calling open(), this fixes that.