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

feat(typography & logos): locally hosted files #797

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

theJohnnyMe
Copy link
Contributor

@theJohnnyMe theJohnnyMe commented Oct 15, 2024

Describe pull-request

The issue our users face is that we load font files via the CDN link set here in Europe, to be precise in Ireland.
That is a problem for users in China where they can not load files outside China.
We have placed the font files as assets in the mono repo and updated the stencil to copy them and make them available when shipping the code.
This way we increase the size of the package from 3.7 to 4.5MB but we make it more stable and independent. It is then really up to the application team to make sure they app is available to China or any other country.
The same issue was with Logotypes used in the Header and Footer components.

Issue Linking:

  • Jira: Add ticket number after CDEP-: CDEP-3530
  • Jira: Add ticket number after CDEP-: CDEP-3085

How to test

  1. Check the preview link and see if Storybook loads fonts and logos currently. Use the network tab of the inspector to confirm font files are loaded and where they are coming from.
  2. Logos are located in Header and Footer component
  3. Run this branch locally.
  4. Do npm run build on packages/core level
  5. Check packages/core/dist/tegel/assets to see if font files can be found.

Checklist before submission

  • I have added unit tests for my changes (if applicable)
  • All existing tests pass
  • I have updated the documentation (if applicable)
  • Not breaking production behavior
  • Behavior available in storybook with documented descriptions (if applicable)
  • npm run build-all without errors

Suggested test steps

  • Browser testing (Chrome, Safari, Firefox)
  • Keyboard operability
  • Interactive elements have labels.
  • Storybook controls
  • Design/controls/props is aligned with other components
  • Dark/light mode and variants
  • Input fields – values should be displayed properly
  • Events

Additional context

I noticed that a lot of tests have failed visually. It seems to me that Docker was sometimes loading different fonts when taking screenshots or it was taking them too early so not all screenshots were 100% the same as when you load the storybook locally.
I have dropped all screenshots and created new ones to compare them here in PR so that others can test it and see if this branch gives a pass on unit tests. Let's see.
Plan is to make beta release, send it over to person from support and ask them to try it out.

Copy link
Contributor

github-actions bot commented Oct 15, 2024

Playwright test results

failed  2 failed
passed  388 passed
skipped  1 skipped

Details

stats  391 tests across 132 suites
duration  53.9 seconds
commit  c867d2d

Failed tests

src/components/dropdown/test/filter/dropdown.e2e.ts › tds-dropdown-filter › toggle dropdown visibility and select option two
src/components/dropdown/test/multiselect-filter/dropdown.e2e.ts › tds-dropdown-multiselect-filter › When focusing on the input it should clear itself

Skipped tests

src/components/table/table/test/expandable-row-autocollapse/expandable-row-autocollapse.e2e.ts › tds-table-expandable-row-autoCollapse › NEEDS FIXING: expanding one row collapses the others when autoCollapse is true

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-797.d3fazya28914g3.amplifyapp.com

@theJohnnyMe theJohnnyMe changed the title feat(typography): locally hosted files feat(typography & svg): locally hosted files Oct 16, 2024
@theJohnnyMe theJohnnyMe changed the title feat(typography & svg): locally hosted files feat(typography & logos): locally hosted files Oct 16, 2024
@MartinPiq
Copy link
Contributor

@theJohnnyMe I think this will not work as you are hoping it to work. By doing this we are just copying the assets into the tegel build and does not mean that the asset files will be automatically hosted by the consuming application. To make it work you would need to point out the files from node modules or copy them over and then host them yourself.

@timrombergjakobsson
Copy link
Contributor

@theJohnnyMe I think this will not work as you are hoping it to work. By doing this we are just copying the assets into the tegel build and does not mean that the asset files will be automatically hosted by the consuming application. To make it work you would need to point out the files from node modules or copy them over and then host them yourself.

Hmm good catch @MartinPiq! I guess the consuming application has to explicitly copy these assets from node_modules to a location that is publicly accessible (e.g., a public or static folder) during the build process? The consuming application must make sure that the fonts are properly copied and served, which requires additional build configuration.
So what could be our options? Maybe have a script or build plugin that is used to copy the fonts from node_modules/@scania/tegel/assets/fonts to a directory that will be publicly served by the consuming application, such as public/assets/fonts for a React app or src/assets/fonts for Angular etc. Both should match as the path in Tegel is:

@font-face {
  font-family: 'Scania Sans Condensed';
  font-weight: bold;
  unicode-range: U+0400-04FF;
  src: url('./assets/fonts/cyrillic/ScaniaSansCYCondensed-Bold.woff') format('woff');

Maybe providing a fallback is valid?

@font-face {
  font-family: 'Scania Sans';
  font-style: italic;
  font-weight: 400;
  src: url('./assets/fonts/latin/ScaniaSans-Italic.woff') format('woff'),
       url('https://cdn.digitaldesign.scania.com/fonts/scania-sans/1.0.0/latin/ScaniaSans-Italic.woff') format('woff');
}

@MartinPiq
Copy link
Contributor

MartinPiq commented Oct 17, 2024

@theJohnnyMe I think this will not work as you are hoping it to work. By doing this we are just copying the assets into the tegel build and does not mean that the asset files will be automatically hosted by the consuming application. To make it work you would need to point out the files from node modules or copy them over and then host them yourself.

Hmm good catch @MartinPiq! I guess the consuming application has to explicitly copy these assets from node_modules to a location that is publicly accessible (e.g., a public or static folder) during the build process? The consuming application must make sure that the fonts are properly copied and served, which requires additional build configuration. So what could be our options? Maybe have a script or build plugin that is used to copy the fonts from node_modules/@scania/tegel/assets/fonts to a directory that will be publicly served by the consuming application, such as public/assets/fonts for a React app or src/assets/fonts for Angular etc. Both should match as the path in Tegel is:

@font-face {
  font-family: 'Scania Sans Condensed';
  font-weight: bold;
  unicode-range: U+0400-04FF;
  src: url('./assets/fonts/cyrillic/ScaniaSansCYCondensed-Bold.woff') format('woff');

Maybe providing a fallback is valid?

@font-face {
  font-family: 'Scania Sans';
  font-style: italic;
  font-weight: 400;
  src: url('./assets/fonts/latin/ScaniaSans-Italic.woff') format('woff'),
       url('https://cdn.digitaldesign.scania.com/fonts/scania-sans/1.0.0/latin/ScaniaSans-Italic.woff') format('woff');
}

A fallback font is a good idea but I think it would be preferable to have the local file as the fallback and the cdn as the primary source. If we don't have a fallback at all this will result in braking changes for all current users of tegel. I do however also think that having a double cdn setup would be the best option, one for within china and one for the rest of the world. This could be another opportunity to get in contact with Porsche as they have a setup that solves this.

I think it would also be an interesting topic to move component code to cdn, similar to how Porsche have done it, as well, this would open up for caching tegel source code between all consuming applications and decreasing build size.

@timrombergjakobsson
Copy link
Contributor

@theJohnnyMe I think this will not work as you are hoping it to work. By doing this we are just copying the assets into the tegel build and does not mean that the asset files will be automatically hosted by the consuming application. To make it work you would need to point out the files from node modules or copy them over and then host them yourself.

Hmm good catch @MartinPiq! I guess the consuming application has to explicitly copy these assets from node_modules to a location that is publicly accessible (e.g., a public or static folder) during the build process? The consuming application must make sure that the fonts are properly copied and served, which requires additional build configuration. So what could be our options? Maybe have a script or build plugin that is used to copy the fonts from node_modules/@scania/tegel/assets/fonts to a directory that will be publicly served by the consuming application, such as public/assets/fonts for a React app or src/assets/fonts for Angular etc. Both should match as the path in Tegel is:

@font-face {
  font-family: 'Scania Sans Condensed';
  font-weight: bold;
  unicode-range: U+0400-04FF;
  src: url('./assets/fonts/cyrillic/ScaniaSansCYCondensed-Bold.woff') format('woff');

Maybe providing a fallback is valid?

@font-face {
  font-family: 'Scania Sans';
  font-style: italic;
  font-weight: 400;
  src: url('./assets/fonts/latin/ScaniaSans-Italic.woff') format('woff'),
       url('https://cdn.digitaldesign.scania.com/fonts/scania-sans/1.0.0/latin/ScaniaSans-Italic.woff') format('woff');
}

A fallback font is a good idea but I think it would be preferable to have the local file as the fallback and the cdn as the primary source. If we don't have a fallback at all this will result in braking changes for all current users of tegel. I do however also think that having a double cdn setup would be the best option, one for within china and one for the rest of the world. This could be another opportunity to get in contact with Porsche as they have a setup that solves this.

I think it would also be an interesting topic to move component code to cdn, similar to how Porsche have done it, as well, this would open up for caching tegel source code between all consuming applications and decreasing build size.

Thinking out loud here, but maybe we could provide a configurable also?

@theJohnnyMe
Copy link
Contributor Author

Thank you both for the great input.
The idea of local files as a fallback is also something that can let me see what can be done.
The team will discuss it and keep working on it next week.

@timrombergjakobsson timrombergjakobsson self-assigned this Oct 21, 2024
Copy link

sonarcloud bot commented Oct 28, 2024

@@ -44,7 +44,6 @@
}

.tds-badge-text {
font-family: var(--tds-font-family-semi-condensed-bold);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed @theJohnnyMe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants