[KED-2984] Remove Kedro-UI webfont usage

[tynandebold]

Description

We're migrating the components we use from Kedro-UI, and this is one of those migrations, though this time it's a deletion and simplification. Here's the relevant ticket (QB-only access).

Development notes

The WebFontLoader library we were using from TypeKit/Google hasn't been updated since May 2017. We weren't really leveraging it's useful options either. We we using it to trigger a Redux action if the web font loaded and even if it didn't:

LoadWebFont({
  active: setFontLoaded,
  inactive: setFontLoaded,
});

The result of that action would be one indicator to Viz if the flowchart should load or not. Since we made no distinction between a successful/unsuccessful font load, I decided to take it out completely. So I've removed the two instances of the Kedro-UI Webfont business that was present in src/components/app/app.js:

import '@quantumblack/kedro-ui/lib/styles/app-no-webfont.css';
import LoadWebFont from '@quantumblack/kedro-ui/lib/utils/webfont.js';

In it's place, I'm loading the Google Font we use in the index.html file:

<link rel="preconnect" href="https://fonts.googleapis.com" />
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
<link
  href="https://fonts.googleapis.com/css2?family=Titillium+Web:wght@400;600&display=swap"
  rel="stylesheet"
/>

And bringing in the few lines of CSS we need for the font styles:

.kedro {
  -webkit-font-smoothing: antialiased;
  font-family: 'Titillium Web', sans-serif;
  font-size: 10px;
  font-weight: 400;
  letter-spacing: 0.1px;
  line-height: 1.4;
}

I've also removed all references that dealt with testing the fontLoaded action/state/etc.

This part of the app is much simpler now, and in a way starts us along a lengthy path moving away from Redux.

QA notes

Check out the branch and see if you notice anything different with the fonts.

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added new entries to the RELEASE.md file
• Added tests to cover my changes

[rashidakanchwala]

I tested this and it works fine. The font is loading correctly. The refactoring makes sense, I wonder why we used WebFonts if we could easily do this. Thanks @tynandebold

[studioswong]

I've tested this and it generally works well, thanks for proposing a much simplier way to load the fonts and stripping the redux stuff.

One main concern is with regards to the loading.graph state, which is also used to trigger the loading spinner during graph calculation ( see my below comment), which is a super quick fix.

[studioswong]

This state cannot be deleted as this is also utilised for the display of the spinner during graph calculations ( refer to selectors/loading.js and the tests outlined under graph.test.js

[tynandebold]

Having this in breaks one of the tests in src/components/app/app.test.js:

The failure:

App › updates the store › but does not override non-pipeline values

It expects "graph": false but receives "graph": true, and I think that's because we not longer prevent the graph from loading based on the status of the font loading. This is why I took it out.

Should I also remove the other relevant getGraphLoading lines from selectors/loading.js or leave the graph property in there and get the tests to pass? I just don't know if it's relevant anymore, that's the thing. It doesn't seem to be.

[limdauto]

Yea it's because the layout calculation is triggered in this test (fontLoaded was what prevented it before, which feels like a hack at best and maybe a coincidence at worst). I wonder if there is anyway to wait for the calculate to be done before assert? Otherwise we can probably modify the expectation to assert for "graph": true

[tynandebold]

Yes, exactly.

I wonder if there is anyway to wait for the calculate to be done before assert?

We could maybe try a timeout or something. That doesn't seem great though. Changing to "graph": true in the loading object solves it. I'd rather do that.

[limdauto]

I wonder if a search for webfont on kedro-viz slack channel would yield anything interesting? I recall seeing it being discussed on slack.

[tynandebold]

Good call. Here's the relevant discussion:

The statement from Richard makes me think we don't need this graph check anymore. He says:

I don’t think it currently does delay booting the whole app, it only delays running a layout calculation and displaying the whole graph. The font is loaded asynchronously while the rest of the app is loaded, but then it doesn’t do layout/graph rendering until after the font is loaded because it’d just have to update it again anyway.

I think we can safely do the layout/graph rendering without messing up the layout because of the font loading. Meaning: we don't have to block it due to fonts.

Plus, now we're using <link rel="preconnect" ... /> which was essentially a wish of his.

[studioswong]

I agree there's no point in blocking the loading of the graph due to fonts ( in the old setup, the graph can still load with the fallback font when, say, there's no internet and is unable to retrieve web fonts, so there's no need to delay it). That said though, loading.graph is not just used for checking the loading of the fonts - it is still used to check if the calculateGraph action is still executing, and hence, display the spinner.

In regards to the test, I might approach it to mocking the dispatch of the action to update the graph loader rather than changing the value in prepareNonPipelineState as it's a once off situation for this test ( to simulate the state when the graph finishes loading) - changing it to graph: true within the prepareNonPipelineState might have further implications to the entire app. ( I know it's a much more painful way, but it's much better to mock for a one off situation rather than changing the state initialisation logic.)

[tynandebold]

I think I follow you but I'm not 100% sure. I've made the changes here by adding a TOGGLE_GRAPH_LOADING test and then removing the non-pipeline values one, because the graph value in that doesn't match anymore due to the removal of the fontLoading. Let me know if this is what you meant.

[studioswong]

just checked the code, and haha nope, this isn't what I meant at all ( esp with the deletion of the test), but regardless it is good to add the test for the reducer.

what I meant initially was in this test is as follows:

test('but does not override non-pipeline values', () => {
      const wrapper = shallow(<App data={demo} />);
      wrapper.setProps({ data: spaceflights });
      // mock the dispatch of the action for graph loading back to false
      expect(getState(wrapper)).toMatchObject(prepareNonPipelineState({}));
    });
  });

alternatively, a quick getaround to this ( other than deleting the test) could be just manually assign the nonpipelinestate object to false:

test('but does not override non-pipeline values', () => {
      // mock the initial pipeline state except for the graph loading state given the state updates immediately on mount
      const mockNonPipelineState = Object.assign(prepareNonPipelineState({}), {loading:{graph: false}})

      const wrapper = shallow(<App data={demo} />);
      wrapper.setProps({ data: spaceflights });
      expect(getState(wrapper)).toMatchObject(mockNonPipelineState);
    });
  });

[tynandebold]

Ohhhh ok, I get it. Please check! Commit is here.

[limdauto]

LGTM

[studioswong]

great work, thanks for pushing through with the test ⭐