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

[K8] Added Inter font files for new theme #102359

Merged
merged 5 commits into from
Jun 17, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jun 16, 2021

... and Updated the Fonts file to accept a themeVersion and only use the old Beta files if v7.

The EUI Amsterdam theme ships using the newest version of the Inter font family which has changed it's name from Inter UI to just Inter. This adds these updated font files, uses the right paths/names depending on the current Kibana theme v7 or v8.

Before
Screen Shot 2021-06-16 at 10 08 39 AM

After
Screen Shot 2021-06-16 at 10 08 52 AM

Checklist

Updated the `Fonts` file to accept a `themeVersion` and only use the old Beta files if `v7`.
@cchaos cchaos requested review from spalger and marius-dr June 16, 2021 14:56
@cchaos cchaos requested a review from a team as a code owner June 16, 2021 14:56
@cchaos cchaos added v7.14.0 release_note:skip Skip the PR/issue when compiling release notes labels Jun 16, 2021
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM when running locally, looks like the visual testing stuff needs updated screenshots, which I'd harvest from the GCS Uploads once the build is complete.

@cchaos
Copy link
Contributor Author

cchaos commented Jun 16, 2021

Thanks @spalger , that's the plan! 🤞

src/core/server/core_app/assets/fonts/readme.md Outdated Show resolved Hide resolved
@@ -24,6 +24,7 @@ export interface RenderingMetadata {
i18n: typeof i18n.translate;
locale: string;
darkMode: boolean;
themeVersion?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly because there's an import of the Font file that doesn't have access to the current theme version.

<Fonts url={uiPublicURL} />

Copy link
Contributor

Choose a reason for hiding this comment

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

Hum, yea. Ideally we'd want to adapt these two calls from security to retrieve the themeVersion setting, however it wouldn't do much, as these pages are rendered when security is enabled and user not authenticated, meaning that it would default to v8 anyway, so I guess having it optional and performing the default/fallback in core's component probably makes sense.

src/core/server/rendering/views/fonts.tsx Outdated Show resolved Hide resolved
@cchaos cchaos requested a review from pgayvallet June 16, 2021 19:45
@spalger
Copy link
Contributor

spalger commented Jun 16, 2021

jenkins, test this

(restarting due to jenkins upgrade)

@cchaos
Copy link
Contributor Author

cchaos commented Jun 16, 2021

jenkins, test this

I don't really understand why this one keeps timing out or how it would be related to this PR, so trying again for the night.

1 failing
     │
     │1)    discover app
     │       discover doc table
     │         legacy
     │           should make the document table scrollable:
     │
     │      timed out waiting for container to be scrollable -- last error: Error: retry.try timeout: TimeoutError: Waiting for element to be located By(css selector, [data-test-subj="field-geo.​coordinates"])
     │ Wait timed out after 10014ms

@spalger
Copy link
Contributor

spalger commented Jun 16, 2021

Hmm, looks like that field is there in the screenshot 🤔
image

Copy link
Member

@marius-dr marius-dr left a comment

Choose a reason for hiding this comment

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

LGTM

@kertal
Copy link
Member

kertal commented Jun 17, 2021

@spalger it should not need to add it, because it's waiting for scrollWidth > clientWidth

[00:20:26] │ debg scrollWidth: 938, clientWidth: 938
[00:20:26] │ debg TestSubjects.moveMouseTo(field-agent)
[00:20:26] │ debg TestSubjects.find(field-agent)
[00:20:26] │ debg Find.findByCssSelector('[data-test-subj="field-agent"]') with timeout=10000
[00:20:27] │ debg TestSubjects.click(fieldToggle-agent)
[00:20:27] │ debg Find.clickByCssSelector('[data-test-subj="fieldToggle-agent"]') with timeout=10000
[00:20:27] │ debg Find.findByCssSelector('[data-test-subj="fieldToggle-agent"]') with timeout=10000
[00:20:30] │ debg scrollWidth: 1020, clientWidth: 938
[00:20:30] │ debg TestSubjects.moveMouseTo(field-bytes)
[00:20:30] │ debg TestSubjects.find(field-bytes)
[00:20:30] │ debg Find.findByCssSelector('[data-test-subj="field-bytes"]') with timeout=10000
[00:20:31] │ debg TestSubjects.click(fieldToggle-bytes)
[00:20:31] │ debg Find.clickByCssSelector('[data-test-subj="fieldToggle-bytes"]') with timeout=10000
[00:20:31] │ debg Find.findByCssSelector('[data-test-subj="fieldToggle-bytes"]') with timeout=10000
[00:20:34] │ debg scrollWidth: 1118, clientWidth: 938

ist should be fine when the agent file is being added, but it just continues?

@kertal
Copy link
Member

kertal commented Jun 17, 2021

@spalger it should not need to add it, because it's waiting for scrollWidth > clientWidth

Ok, I think I know what could work, scrollWidth: 1020, clientWidth: 938 in the test are not converted to numbers

'1020' > '938' = false

return scrollWidth > clientWidth;

so it should be

return Number(scrollWidth) > Number(clientWidth);

Then the check should work, seems this PR changed the widths of the columns

@spalger
Copy link
Contributor

spalger commented Jun 17, 2021

Looks like @kertal was right, but got snagged by a flaky "unit" test

@cchaos
Copy link
Contributor Author

cchaos commented Jun 17, 2021

I was wondering if that one for sure is flakey. But I'll update from master anyway and get a new CI going.

@elasticmachine merge upstream

@cchaos
Copy link
Contributor Author

cchaos commented Jun 17, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@kertal
Copy link
Member

kertal commented Jun 17, 2021

🥳

@cchaos cchaos merged commit e483351 into elastic:master Jun 17, 2021
cchaos added a commit to cchaos/kibana that referenced this pull request Jun 17, 2021
Updated the `Fonts` file to accept a `themeVersion` and only use the old Beta files if `v7`.
# Conflicts:
#	test/functional/screenshots/baseline/tsvb_dashboard.png
#	test/interpreter_functional/screenshots/baseline/metric_multi_metric_data.png
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 18, 2021
…ets-tab

* 'master' of github.com:elastic/kibana: (93 commits)
  [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch (elastic#102506)
  [Lens] Fix wrong error detection on transition to Top values operation (elastic#102384)
  [ML] Anomaly detection job custom_settings improvements (elastic#102099)
  [Cases] Route: Get all alerts attach to a case (elastic#101878)
  Fixes wrong list exception type when creating endpoint event filters list (elastic#102522)
  remove search bar that's not working yet (elastic#102550)
  Migrated Ingest Node Pipeline Functional Tests to use test_user (elastic#102409)
  [Maps] clean up feature editing name space to avoid conflicts with layer settings editing (elastic#102516)
  [canvas] Refactor Storybook from bespoke to standard configuration (elastic#101962)
  [Security Solution] adds wrapSequences method (RAC) (elastic#102106)
  [FTR] Stabilize SSLP functional tests (elastic#102553)
  [K8] Added `Inter` font files for new theme (elastic#102359)
  [Workplace Search] Convert Groups pages to new page template (elastic#102449)
  [DOC] Add experimental disclaimer to rollup jobs (elastic#95624)
  [Security Solution][Endpoint] Suppress some of the jest console.error noise created by endpoint list middelware (elastic#102535)
  [Fleet] Improve performance of Fleet setup (elastic#102219)
  [Alerting] Add event log entry when a rule starts executing (elastic#102001)
  [Fleet] Update docker image of registry used in integration tests (elastic#101911)
  [Asset Management] Osquery telemetry updates (elastic#100754)
  Converts saved object tagging to new management layout (elastic#102284)
  ...

# Conflicts:
#	x-pack/plugins/fleet/kibana.json
cchaos added a commit that referenced this pull request Jun 20, 2021
* [K8] Added `Inter` font files for new theme (#102359)

Updated the `Fonts` file to accept a `themeVersion` and only use the old Beta files if `v7`.
# Conflicts:
#	test/functional/screenshots/baseline/tsvb_dashboard.png
#	test/interpreter_functional/screenshots/baseline/metric_multi_metric_data.png

* Update baseline snapshot

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@cchaos cchaos deleted the k8_fix/font-family branch March 11, 2022 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants