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

Update DefaultFieldFactory.java #54

Closed
wants to merge 1 commit into from
Closed

Update DefaultFieldFactory.java #54

wants to merge 1 commit into from

Conversation

adihubba
Copy link

@adihubba adihubba commented Jun 3, 2016

In the Vaadin book there is a suggestion to extend from this class to implement an own factory. There is absolutely no reason why the two methods are static. If you remove the two static modifiers, it's a lot easier to extend from this class.

In the Vaadin book there is a suggestion to extend from this class to implement an own factory. There is absolutely no reason why the two methods are static. If you remove the two static modifiers, it's a lot easier to extend from this class.
@elmot
Copy link
Contributor

elmot commented Nov 28, 2016

Please follow the https://github.com/vaadin/vaadin/blob/master/CONTRIBUTING.md for code contribution. If you are implementing the patch for Vaadin 7, please push your change for "7.7" branch, not master, i.e.
use git push gerrit HEAD:refs/for/7.7. Also don't forget to file a ticket in trac and note its number in the commit message. I close the push request here.

@elmot elmot closed this Nov 28, 2016
@hesara
Copy link
Contributor

hesara commented Dec 5, 2016

@adihubba Thanks for the PR! Sorry us being a bit in the middle of our migration from Gerrit to GitHub, and not having the proper time to process this pull request.

Our contribution guide now looks quite different than last week. We now work completely in GitHub, and accept outside PR’s here as well 🎉

The tickets from our previous system (Trac) are also being migrated to GitHub Issues, but that’s probably going to happen later this week at the earliest.

So, coming back to this particular PR, it concerns DefaultFieldFactory which is actively used in Vaadin 7 but only in the compatibility package in Vaadin 8. Thus, I believe this change should probably go to the 7.7 branch in case it doesn't break API compatibility too much.

Could you please make a PR with these changes picked on the latest 7.7 branch, formulate the commit message as recommended in https://github.com/vaadin/framework/blob/master/CONTRIBUTING.md and accept the contribution agreement on GitHub (it will show up on the PR page after your next push). We could then give it a proper review and hopefully integrate the changes.

@adihubba adihubba deleted the patch-1 branch December 5, 2016 16:19
@tsuoanttila tsuoanttila added this to the Invalid milestone Apr 30, 2018
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.

None yet

4 participants