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 field Display ID in Returns search #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add field Display ID in Returns search #93

wants to merge 1 commit into from

Conversation

ntorres1986
Copy link

No description provided.

@jonesde
Copy link
Member

jonesde commented Feb 19, 2019

This is a small change but even for this IP clearance is required by 'signing' the AUTHORS file with a git commit under your account submitted through a pull request (this pull request can be updated after you've made the change in your forked repository). For more details please see the Intellectual Property section of the Issue and Pull Request Guide:

https://www.moqui.org/m/docs/moqui/Issue+and+Pull+Request+Guide

The change looks fine. While reviewing this I noticed it is based on the XML just above this change for the returnId field which is older code. For ID fields the case-insensitive sort is less efficient and generally not needed (and sometimes can't be done on distinct queries) so might as well be 'true' instead of 'case-insensitive'. Also now that the form-list uses header-dialog="true" the text-line size can be larger. In other words it was set to '10' on returnId because that was originally a find field in a table header row. Now that the header-dialog is supported that approach is not used so much (especially for more complex form-list forms), partly for this reason.

Another factor to look at when adding a new field to a form-list is whether the additional column starts making it too wide and fields need to be hidden by default (viewable through select-columns=true) and/or field stacking within a form-list-column. For a smaller change like this that's not a big deal, I'll review it after merge.

@jonesde
Copy link
Member

jonesde commented Sep 20, 2019

Please sign the AUTHORS file or I'll close this pull request soon, it has been open with no response for a few months now.

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

Successfully merging this pull request may close these issues.

3 participants