-
Notifications
You must be signed in to change notification settings - Fork 2
Sample finder polish #754
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
Sample finder polish #754
Conversation
…ype name to column name
…fb_sampleFinderPolishSusan
| * Added getLabKeySqlWhere util | ||
| * Added InExpAncestorsOfFilterType and InExpDescendantsOfFilterType | ||
| * Enable non-text fields for EntityFieldFilterModal (Sample Finder) | ||
| * Add "showing all generations" message |
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.
hmmm, should this message be up with your 2.138.X notes?
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.
Oops! Thanks. Actually just removing this as no longer relevant.
| <div className="pull-left"> | ||
| <div className="secondary-text">{capitalizeFirstChar(entityDataType.nounAsParentSingular)}</div> | ||
| <div className="primary-text">{schemaQuery.queryName}</div> | ||
| <div className="primary-text">{dataTypeDisplayName}</div> |
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.
any reason to fall back on using the schemaQuery.queryName here and below (i.e. dataTypeDisplayName ?? schemaQuery.queryName)? I assume not, but just checking since it looks like dataTypeDisplayName is an optional prop.
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.
Yes, good idea. In practice, currently, it will never not be defined, but that's a better fallback.
| padding-bottom: 5px; | ||
| } | ||
|
|
||
| .filter-results__message { |
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.
is this still needed with the move of the "all generations" text to the page subtitle?
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.
Removed.
Rationale
We're releasing Sample Finder in 22.3 and just need to polish a few parts of it before we do that.
Related Pull Requests
Changes
containerFilterproperty toEntityDataTypemodel