Skip to content
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

Add missing stuff #14

Merged
merged 4 commits into from
Mar 10, 2013
Merged

Add missing stuff #14

merged 4 commits into from
Mar 10, 2013

Conversation

ndarilek
Copy link
Contributor

@ndarilek ndarilek commented Mar 2, 2013

Added assorted bits that I'd found myself missing:

AbstractField.valid

AbstractSelect.setItemCaption: Don't know if this should be AbstractSelect.itemCaption, but it felt like I should prepend "set" because it isn't a single variable. Maybe it'd be better modeled as a Map[Any, String] but I don't know enough about Scaladin internals to pull that off.

Select: I was surprised that there was a NativeSelect component but not a plain Select.

@henrikerola
Copy link
Owner

ComboBox should be used instead of deprecated Select.

@ndarilek
Copy link
Contributor Author

ndarilek commented Mar 9, 2013

So is removing Select all that is needed for this to be merged? If so, I can remove the commit.

@henrikerola
Copy link
Owner

Yes, and other corrections:

  • valid should be in the Validatable trait
  • Add also getItemCaption

After these it's ok for merge.

@ndarilek
Copy link
Contributor Author

ndarilek commented Mar 9, 2013

OK, almost done. One question, how do you want getItemCaption/setItemCaption named since they aren't direct analogues to variables?

For the get case, there appears to be precedent for methods stripping the get:

def selected(itemId: Any) = p.isSelected(itemId)

So I could do:

def itemCaption(item: Any): String = p.getItemCaption(item)

But I'm unclear as to weather:

def itemCaption(item: Any, caption: String) = p.setItemCaption(item, caption)

is as clear/in keeping with style.

Thanks.

@henrikerola
Copy link
Owner

getItemCaption/setItemCaption is the correct form. I know that there are lots of those non-setter/getter methods that are named like scala setter/getter and that's wrong, they should be fixed.

@ndarilek
Copy link
Contributor Author

All of the above changes are made and on the branch.

henrikerola pushed a commit that referenced this pull request Mar 10, 2013
AbstractSelect.get/setItemCaption and Validatable.valid
@henrikerola henrikerola merged commit 824ea67 into henrikerola:3.0 Mar 10, 2013
@henrikerola
Copy link
Owner

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants