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.

Sign up for GitHub

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

Jump to bottom

[App Search] First cut of the Relevance Tuning UI #90621

Merged
merged 16 commits into from
Feb 18, 2021
Merged

[App Search] First cut of the Relevance Tuning UI #90621

merged 16 commits into from
Feb 18, 2021

Conversation

JasonStoltz
Copy link
Member

@JasonStoltz JasonStoltz commented Feb 8, 2021 •
edited
Loading

Summary

This is Part 4 of Relevance Tuning.

This PR migrates the UI for the Relevance Tuning page from ent-search to Kibana. It migrates the entire form excluding the individual forms for boosts. Those will come in a follow up PR.

Kibana:
latest

ent-search:
latest

Responsive:
thing

Checklist

Delete any items that are not applicable to this PR.

@JasonStoltz JasonStoltz added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 8, 2021
JasonStoltz
JasonStoltz commented Feb 8, 2021
View reviewed changes
...c/applications/app_search/components/relevance_tuning/relevance_tuning_form/value_badge.scss
@@ -0,0 +1,23 @@
.relevanceTuningForm {
.valueBadge {
Copy link
Member Author

@JasonStoltz JasonStoltz Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@daveyholler (cc @constancecchen and @byronhulcher)

I have axe failures for the follow elements:
Relevance_Tuning_-_pokemon_-_Engines_-_App_Search_-_Elastic

In the image above, you can see examples of the "enabled" and "disabled" styles of this component.

I made this component up, basically. It it my take on what we had in ent-search, just translated to Kibana.

The background color (tintOrShade($euiColorVis1, 90%, 70%);) I chose specifically to match the background of EuiToken. We show this component beside EuiToken elements, so I thought it would make sense to match the styles a bit.

The foreground color ($euiColorVis1) matches the color I chose for the EuiTokens. Note that in the Eui docs for EuiToken (https://elastic.github.io/eui/#/display/icons) it recommends using a "vis" color, which is why I picked this odd looking color:

For best results use one of the vis color names (or 'gray').

However, as you can see, axe complains about the color contrast.

The disabled styles are also completely made up. They also fail.

I'm looking for some advice on what colors to use here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have super strong thoughts here, if we're waiting for a total component rehaul I'm inclined to just leave this as-is with your current interpretation and let Davey handle it in the redesign :)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point.

@JasonStoltz JasonStoltz changed the title Relevance tuning 4 [App Search] First cut of the Relevance Tuning UI Feb 9, 2021
@byronhulcher
Copy link
Contributor

This seems pretty good so far, I left a few comments, mostly questions to ponder. I look forward to taking another look at this after it's been rebased post-Part 3

@JasonStoltz JasonStoltz added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 11, 2021
@JasonStoltz
Copy link
Member Author

I rebased with master and I'm now marking this as Ready for review. I also added a commit to add coverage for a couple of missing branches.

@JasonStoltz JasonStoltz marked this pull request as ready for review February 11, 2021 17:02
@JasonStoltz JasonStoltz requested a review from a team February 11, 2021 17:02
cee-chen
cee-chen reviewed Feb 12, 2021
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Apologies for the lateness on this Jason - here's a very quick first pass of comments purely from skimming the code, I haven't pulled this down yet to examine UI/UX/DOM/QA etc. but will do another deeper dive review on Monday most likely.

...ions/app_search/components/relevance_tuning/relevance_tuning_form/relevance_tuning_form.scss Show resolved Hide resolved
..._search/public/applications/app_search/components/relevance_tuning/relevance_tuning_logic.ts
Comment on lines +507 to +511
if (optionType === 'operation') {
updatedBoosts[boostIndex][optionType] = value as BoostOperation;
} else {
updatedBoosts[boostIndex][optionType] = value as BoostFunction;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, I legit have no idea - does updatedBoosts[boostIndex][optionType] = value as BoostOperation | BoostFunction work or nah? No worries if not, maybe @byronhulcher might also have some ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

No dice with that. Which is weird, because I can do something similar in TS Playground. Not the first time where I could do something in TS Playground but not here: https://www.typescriptlang.org/play?ssl=1&ssc=1&pln=17&pc=71#code/KYDwDg9gTgLgBMAdgVwLZwGLMQYxgSwkQEMAbAIQggGcYtcCi4BvAWACg44BxY5a6vmKI4AXjgByAOZ8BQxBIA0HLgFFwRJATJjJoSIi1DSSlXAAy+Q8Si6JpK8Bun2AXw4d90eEjRwAClAQIPio+DAAnpQ0dNh4hCJsnDyygsJ2pBAyUOEAFmE4LmoahojapHZepeVFFo42GfVQLu7sHJFgwHDRtPTxTOJ9jCQUVL1xw3AAPgFBIWGRPbEMCQDcHuykwPDEAFyYEwlkS0MJ07PBoeFRY8v9iOtt7MQZWTZ5BRJwxNQHK0THW6nJgzQKXBY3GLAh5AA

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, no worries. It doesn't excessively bother me to be adding branching logic for the sake of typing but it does feel a bit weird. Leaving this uncollapsed in case anyone has any other ideas or if we come back to this in the future at some point, but no change requested

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely, ty.

..._search/public/applications/app_search/components/relevance_tuning/relevance_tuning_logic.ts Show resolved Hide resolved
...ic/applications/app_search/components/relevance_tuning/relevance_tuning_form/value_badge.tsx Show resolved Hide resolved
...nents/relevance_tuning/relevance_tuning_form/relevance_tuning_item_content/weight_slider.tsx Show resolved Hide resolved
...tions/app_search/components/relevance_tuning/relevance_tuning_form/relevance_tuning_form.tsx
}
paddingSize="s"
>
<RelevanceTuningItemContent
Copy link
Contributor

@cee-chen cee-chen Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

[Not a change request, just a general sound for opinions] How do we feel about the name of this? No worries if we're just straight up porting it from ent-search and don't want to think about it, but maybe RelevanceTuningField vs ItemContent is both more descriptive & succinct? 🤷

...s/relevance_tuning/relevance_tuning_form/relevance_tuning_item_content/boosts/boost_item.tsx Show resolved Hide resolved
...nents/relevance_tuning/relevance_tuning_form/relevance_tuning_item_content/boosts/boosts.tsx Outdated Show resolved Hide resolved
...nents/relevance_tuning/relevance_tuning_form/relevance_tuning_item_content/boosts/boosts.tsx Show resolved Hide resolved
..._tuning/relevance_tuning_form/relevance_tuning_item_content/boosts/get_boost_summary.test.ts Show resolved Hide resolved
@JasonStoltz JasonStoltz requested a review from cee-chen February 17, 2021 15:30
cee-chen
cee-chen approved these changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Changes and QA look great - thanks Jason for the awesome UI/UX work here and for super thorough unit tests as always!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1225 1275 +50

Async chunks

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

id before after diff
enterpriseSearch 1.9MB 1.9MB +28.7KB

History

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

@JasonStoltz JasonStoltz merged commit 8c9eaa2 into elastic:master Feb 18, 2021
@JasonStoltz JasonStoltz deleted the relevance-tuning-4 branch February 18, 2021 13:07
@kibanamachine
Copy link
Contributor

💔 Backport failed

❌ 7.13: The branch "7.13" is invalid or doesn't exist

To backport manually, check out the target branch and run:
node scripts/backport --pr 90621

@JasonStoltz JasonStoltz mentioned this pull request Feb 18, 2021
Merged
JasonStoltz added a commit to JasonStoltz/kibana that referenced this pull request Feb 18, 2021
@JasonStoltz
[App Search] First cut of the Relevance Tuning UI (elastic#90621)
ffd6028
JasonStoltz added a commit that referenced this pull request Feb 18, 2021
@JasonStoltz
[App Search] First cut of the Relevance Tuning UI (#90621) (#91851)
2b0834a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

@daveyholler daveyholler Awaiting requested review from daveyholler

@byronhulcher byronhulcher Awaiting requested review from byronhulcher

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JasonStoltz @byronhulcher @kibanamachine @cee-chen