-
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
Create new Salesforce element types (closes #12) #14
base: master
Are you sure you want to change the base?
Conversation
:( Lemme work on that |
|
||
@Override | ||
public void toggle() { | ||
click(); |
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 seems trivial, but I'd recommend calling toggle
on the nestedCheckbox
as well to be consistent. It is [remotely] conceivable that a darcy-web implementation may do something differently with toggle to make sure the checkbox is indeed toggled rather than just clicking on it. I'm reaching, but, it's an easy though very very very tiny win.
List<String> values = new ArrayList<String>( | ||
asList(nestedText.getText().split(";"))); | ||
values.removeAll(asList(null, "")); | ||
return values.toArray(new String[values.size()]); |
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 an array type here? I prefer collections.
Can you describe what this element is a little bit? I'm guessing it shouldn't implement MultiSelect
because you cannot select options, though it does display some selected options?
If that is the case, for consistency, I would like to maybe name this getSelectedOptions
and return a List<String>
, but, I'd like to have a better idea of how the element works exactly.
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's the non-editable output of a picklist/select. If the select had multiple enabled, then the output is a semicolon separated list. I was just attempting to save the hassle of parsing that out of getText() if you happened to need it for some reason. Using a collection is easy enough. Guess I should have looked the other getOptions(), getSelectedOptions().
* on a Salesforce object. Takes the locator returned from BySalesforce | ||
* and finds the input tag nested below. | ||
* | ||
* @param locator Locator returned from BySalesforce |
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 would be more specific to the dom here. This helps when even implementing things in BySalesforce
. For instance, if we say, "Accepts a locator for the parent element of the checkbox image," then that influences what locator code we write in BySalesforce
. Otherwise, if we say, "uses what BySalesforce returns" it's a kind of chicken and egg problem :) (plus BySalesforce
is intended to house as many locators as make sense). Looking at BySalesforce
, I think we need to similarly improve the documentation: what exactly is being located from a DOM point of view?
After much anticipation....