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

[App Search] Meta engines schema view #100087

Merged
merged 4 commits into from
May 14, 2021
Merged

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented May 13, 2021

Summary

Meta engines do not have documents/schema themselves, so they instead display an overview of the source engine schema as well as showing any conflicts in types for source engines.

Screencaps

meta engines conflicts

New in Kibana / different from ent-search

  1. The inactive fields table got a huge cleanup/redesign thanks to the ineffable @daveyholler!
    • Before:
    • After:
    • The code is much cleaner with less one-off styling, and handles larger amounts of data better in terms of visual rhythm.
  2. I made engine names links to the source engine schema pages. This should hopefully make it significantly easier to fix type conflicts.
    • Davey has some even cooler ideas here about potentially adding a new button or piece of logic to update/set all source engines to a certain type all at once. Not in this PR, but maybe before 7.14.

QA

  • Ensure you have a trial/platinum license
  • Create at least 2 source engines
  • Add just 1 source engine to a new meta engine
  • Confirm the Schema page shows only the active fields table and no conflicts callout or table
  • Add another source engine with a conflicting field type
  • Confirm the Schema page now shows a callout and the inactive fields table
  • Fix the schema conflict and go back to the meta engine schema
  • Confirm the conflict callout is gone

Checklist

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
    • NB: No critical issues, but a Links with the same name have a similar purpose warning is flagged for manual review, which is intentional given that links are within tables.
  • This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
    • NB: All of our schema tables are kind of janky on phones unfortunately. Might have to be something we revisit later
  • This was checked for cross-browser compatibility

cee-chen added 3 commits May 13, 2021 13:37
- Used for listing source engines
- New in Kibana: now links to source engine schema pages for easier schema fixes!
@cee-chen cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels May 13, 2021
@cee-chen cee-chen requested a review from a team May 13, 2021 20:57
@cee-chen
Copy link
Contributor Author

This is the last schema view!!! With this PR, I think all our views but crawler are migrated 🤞 💦

Comment on lines +45 to +57
{Object.entries(conflictingFields).map(([fieldName, conflicts]) =>
Object.entries(conflicts).map(([fieldType, engines], i) => {
const isFirstRow = i === 0;
return (
<EuiTableRow key={`${fieldName}-${fieldType}`}>
{isFirstRow && (
<EuiTableRowCell
rowSpan={Object.values(conflicts).length}
data-test-subj="fieldName"
>
<code>{fieldName}</code>
</EuiTableRowCell>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this isn't too difficult to grok code-wise, but the goal of the first row check and rowSpan behavior is to essentially render this:

TBH it's still less code than the original ent-search view, purely by having less custom design etc :)

Comment on lines +34 to +39
<EuiLinkTo
to={generateEncodedPath(ENGINE_SCHEMA_PATH, { engineName })}
data-test-subj="displayedEngine"
>
{engineName}
</EuiLinkTo>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully y'all agree the source engine links are helpful! Very pleased it's such an easy thing for us to do now in Kibana.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very helpful, IMO.

<EuiButtonEmpty size="xs" flush="both" data-test-subj="hiddenEnginesToggle">
+{hiddenEngines.length}
</EuiButtonEmpty>
</EuiToolTip>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot of truncation/tooltip behavior:

'xpack.enterpriseSearch.appSearch.engine.schema.metaEngine.conflictsCalloutTitle',
{
defaultMessage:
'{conflictingFieldsCount, plural, one {# field is} other {# fields are}} not searchable',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure this i18n is correct. Does anyone know for sure how to handle is vs. are pluralization in i18n?... 😬

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't. But this looks good sufficient to me. Since the entire message is in the i18n string I think the translator has enough control to make this work in other languages.

Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @constancecchen. I really appreciate the cleanup of the field types conflicts, it looks 10x better than before. The code is super clean and modular as well.

I dropped a couple of comments about i18n, I'd just merge this PR as is though, we can circle back to those later if we decide it's warranted.

data-test-subj="hiddenEnginesTooltip"
>
<EuiButtonEmpty size="xs" flush="both" data-test-subj="hiddenEnginesToggle">
+{hiddenEngines.length}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we i18n this?

I didn't test this, but would it be something like...

Suggested change
+{hiddenEngines.length}
{i18n.translate(
'xpack.enterpriseSearch.appSearch.engine.schema.metaEngine.additionalEnginesCount',
{ defaultMessage: '+{count, number}', values: { count: hiddenEngines.length } }
})

I'm actually unsure of what the best practice is here. If you're unsure also maybe we can inquire with #kibana-localization

Or perhaps just a FormattedNumber would work here. I guess it depends on whether the + is significant to translate to other languages.


const displayedEngines = engines.slice(0, cutoff);
const hiddenEngines = engines.slice(cutoff);
const SEPARATOR = ', ';
Copy link
Member

@JasonStoltz JasonStoltz May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually wonder if this should be i18ned as well.

I did some digging in the localization channel in slack, I think if you pass an array of values as a value to IntlMessageFormat i18n as an array value, it could do the concatenation for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the difficulty here is that I'm using SEPARATOR in various places and not just as a joined array (e.g. conditionally if the tooltip truncation kicks in, in a .map because we're using EuiLinkTo for the engine names, etc. 🤔

To be honest, I think a better approach might be to use something like <EuiBadge> or some other EUI list component to list out engines in any case... Maybe just copy what we do for analytics tags:

Our tag truncation component (link) also uses and X more text instead of a "+X" copy. We avoid all the i18n shenanigans with commas AND the + with that approach 🤔

@daveyholler what do you think?... would engine names in badges be weird?

@@ -14,7 +14,9 @@ import { i18n } from '@kbn/i18n';

import { FlashMessages } from '../../../../shared/flash_messages';
import { Loading } from '../../../../shared/loading';
import { DataPanel } from '../../data_panel';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, nice, looks like you created this for another view and were able to reuse? That's great.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha all credit goes to @daveyholler for this component! 🎉

'xpack.enterpriseSearch.appSearch.engine.schema.metaEngine.conflictsCalloutTitle',
{
defaultMessage:
'{conflictingFieldsCount, plural, one {# field is} other {# fields are}} not searchable',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't. But this looks good sufficient to me. Since the entire message is in the i18n string I think the translator has enough control to make this work in other languages.

…h/components/schema/components/truncated_engines_list.tsx

Co-authored-by: Jason Stoltzfus <[email protected]>
@cee-chen
Copy link
Contributor Author

Going to go ahead and merge for now / to get this done before GAH, but would definitely like to revisit i18n more broadly in the future!

@cee-chen cee-chen enabled auto-merge (squash) May 14, 2021 21:42
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1388 1391 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 2.1MB 2.1MB +8.1KB
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 4 2 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 5 3 -2
licensing 18 15 -3
monitoring 109 56 -53
total -73

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cee-chen cee-chen merged commit 091ca43 into elastic:master May 14, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 14, 2021
* Set up TruncatedEnginesList component

- Used for listing source engines
- New in Kibana: now links to source engine schema pages for easier schema fixes!

* Add meta engines schema active fields table

* Render meta engine schema conflicts table & warning callout

* Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/components/truncated_engines_list.tsx

Co-authored-by: Jason Stoltzfus <[email protected]>

Co-authored-by: Jason Stoltzfus <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@cee-chen cee-chen deleted the schema-6 branch May 14, 2021 22:35
kibanamachine added a commit that referenced this pull request May 15, 2021
* Set up TruncatedEnginesList component

- Used for listing source engines
- New in Kibana: now links to source engine schema pages for easier schema fixes!

* Add meta engines schema active fields table

* Render meta engine schema conflicts table & warning callout

* Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/components/truncated_engines_list.tsx

Co-authored-by: Jason Stoltzfus <[email protected]>

Co-authored-by: Jason Stoltzfus <[email protected]>

Co-authored-by: Constance <[email protected]>
Co-authored-by: Jason Stoltzfus <[email protected]>
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 25, 2021
* Set up TruncatedEnginesList component

- Used for listing source engines
- New in Kibana: now links to source engine schema pages for easier schema fixes!

* Add meta engines schema active fields table

* Render meta engine schema conflicts table & warning callout

* Update x-pack/plugins/enterprise_search/public/applications/app_search/components/schema/components/truncated_engines_list.tsx

Co-authored-by: Jason Stoltzfus <[email protected]>

Co-authored-by: Jason Stoltzfus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants