[LOOM-1377][multiple-components]: remove unrequired ts ignores and address any typing issues #3392

mungodewar · 2024-04-26T15:33:14Z

Update packages to consume new typed svg and foundations.

Remember to include the following changes:

Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
README.md (If you have created a new component)
Component README.md
Tests
Storybook examples created/updated
Type declaration (.d.ts) files updated
For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

mungodewar · 2024-04-26T15:33:42Z

gulpfile.js/bpk-component-icon/gulpfile.babel.js

@@ -22,7 +22,7 @@ const ICONS_FOLDER_PATH = './node_modules/@skyscanner/bpk-svgs/dist/js/icons';

 gulp.task('copy', () =>
  gulp
-    .src(`${ICONS_FOLDER_PATH}/**/*.js`)
+    .src(`${ICONS_FOLDER_PATH}/**/*`)


We need both the .d.ts file and the .jsx file.

github-actions · 2024-04-26T16:17:38Z

	Warnings
⚠️	Package source files (e.g. `packages/package-name/src/Component.tsx`) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against f527d6f

github-actions · 2024-04-26T16:17:56Z

Visit https://backpack.github.io/storybook-prs/3392 to see this build running in a browser.

github-actions · 2024-04-26T16:25:56Z

Visit https://backpack.github.io/storybook-prs/3392 to see this build running in a browser.

github-actions · 2024-04-29T09:21:44Z

Visit https://backpack.github.io/storybook-prs/3392 to see this build running in a browser.

olliecurtis

A couple of comments. I have a concern on the level of changes to the snaps showing much different path values now. Do we know why that is?

olliecurtis · 2024-04-29T12:55:36Z

packages/bpk-component-accordion/src/__snapshots__/BpkAccordionItem-test.js.snap

                xmlns="http://www.w3.org/2000/svg"
              >
                <path
-                  d="M19.113 8.095a1.496 1.496 0 010 2.008l-6.397 5.948a1 1 0 01-1.358.003l-6.532-6.01a1.427 1.427 0 01.138-1.949 1.572 1.572 0 011.997-.103l5.078 4.638 4.97-4.535a1.72 1.72 0 012.104 0z"
+                  d="M19.113 8.095a1.496 1.496 0 0 1 0 2.008l-6.397 5.948a1 1 0 0 1-1.358.003l-6.532-6.01a1.427 1.427 0 0 1 .138-1.949 1.57 1.57 0 0 1 1.997-.103l5.078 4.638 4.97-4.535a1.72 1.72 0 0 1 2.104 0"


Is a path change like this to be a concern?

No concern, the diff is within an a command: https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/d#path_commands for drawing arcs. The previous parser/generator concatenated the large-arc-flag and sweep-flag (either 0 or 1) and the new parser/generator does not. Both are valid and do not change the resulting svg viewed.

In addition I can see that some path values (1.572) have been shortened (1.57). My guess is that this is an optimisation step performed during generation & accuracy to 3 decimal places does not increase visual clarity so it is optimised away.

olliecurtis · 2024-04-29T12:55:54Z

packages/bpk-component-accordion/src/__snapshots__/BpkAccordionItem-test.js.snap

                xmlns="http://www.w3.org/2000/svg"
              >
                <path
-                  d="M11.25 7.5a2.25 2.25 0 100-4.5 2.25 2.25 0 000 4.5zm-3.588-.866a1.5 1.5 0 01-.358 2.09C5.597 9.933 4.5 11.89 4.5 14.092V19.5a1.5 1.5 0 11-3 0v-5.41c0-3.227 1.614-6.076 4.071-7.814a1.5 1.5 0 012.091.358zm7.176 0a1.5 1.5 0 012.09-.359C19.387 8.014 21 10.864 21 14.091v1.788l.44-.44a1.5 1.5 0 012.12 2.122l-3 3a1.5 1.5 0 01-2.12 0l-3-3a1.5 1.5 0 012.12-2.122l.44.44V14.09c0-2.201-1.097-4.159-2.804-5.366a1.5 1.5 0 01-.358-2.091z"
+                  d="M11.25 7.5a2.25 2.25 0 1 0 0-4.5 2.25 2.25 0 0 0 0 4.5m-3.588-.866a1.5 1.5 0 0 1-.358 2.09C5.597 9.933 4.5 11.89 4.5 14.092V19.5a1.5 1.5 0 1 1-3 0v-5.41c0-3.227 1.614-6.076 4.071-7.814a1.5 1.5 0 0 1 2.091.358m7.176 0a1.5 1.5 0 0 1 2.09-.359C19.387 8.014 21 10.864 21 14.091v1.788l.44-.44a1.5 1.5 0 0 1 2.12 2.122l-3 3a1.5 1.5 0 0 1-2.12 0l-3-3a1.5 1.5 0 0 1 2.12-2.122l.44.44V14.09c0-2.201-1.097-4.159-2.804-5.366a1.5 1.5 0 0 1-.358-2.091z"


#3392 (comment)

olliecurtis · 2024-04-29T12:58:15Z

packages/bpk-component-image/src/BpkImage-test.tsx

@@ -43,7 +43,7 @@ describe('BpkImage', () => {
      <BpkImage
        altText="image description"
        aspectRatio={816 / 544}
-        style={{ width: spacingSm }}
+        style={{ width: spacings.onePixelRem }}


Could we just put a bigger value in here? Concern here is that the tests may not catch any issues if its only 1rem in width

This test isn't visual, only testing that user values are placed onto the correct area of DOM, I can absolutely increase the value, however, it will provide no increase to the validity of the test.

olliecurtis · 2024-04-29T12:59:56Z

package.json

+    "@skyscanner/bpk-foundations-web": "^17.11.0",
+    "@skyscanner/bpk-svgs": "^19.3.0",


We also need to bump these versions inside the packages/ folder so consumers will also be able to get these changes without issue too

github-actions · 2024-04-29T14:38:30Z

Visit https://backpack.github.io/storybook-prs/3392 to see this build running in a browser.

github-actions · 2024-04-29T15:09:28Z

Visit https://backpack.github.io/storybook-prs/3392 to see this build running in a browser.

metalix2

Nice looks good! I was investigating more about SVGs yesterday so it's quite good timing for this review.

…dress any typing issues (#3392) * remove unrequired ts ignores and address any typing issues * update test and snap * update lock file * update to all.tsx * update snaps * update inside packages too * update snaps

remove unrequired ts ignores and address any typing issues

ecd64ee

mungodewar added the patch Patch production bug label Apr 26, 2024

mungodewar commented Apr 26, 2024

View reviewed changes

mungodewar added 2 commits April 26, 2024 17:11

update test and snap

7fcc83e

update lock file

5f1c38f

update to all.tsx

dd94ee7

update snaps

9c21b8c

mungodewar marked this pull request as ready for review April 29, 2024 10:20

olliecurtis requested changes Apr 29, 2024

View reviewed changes

update inside packages too

9f8fe15

update snaps

f527d6f

metalix2 approved these changes Apr 30, 2024

View reviewed changes

olliecurtis approved these changes Apr 30, 2024

View reviewed changes

mungodewar merged commit 96ebc1b into main Apr 30, 2024
9 checks passed

mungodewar deleted the loom-1377-update-icons-and-foundations branch April 30, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LOOM-1377][multiple-components]: remove unrequired ts ignores and address any typing issues #3392

[LOOM-1377][multiple-components]: remove unrequired ts ignores and address any typing issues #3392

mungodewar commented Apr 26, 2024 •

edited

mungodewar Apr 26, 2024

github-actions bot commented Apr 26, 2024 •

edited

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 29, 2024

olliecurtis left a comment

olliecurtis Apr 29, 2024

mungodewar Apr 29, 2024

olliecurtis Apr 29, 2024

mungodewar Apr 29, 2024

olliecurtis Apr 29, 2024

mungodewar Apr 29, 2024

olliecurtis Apr 29, 2024

github-actions bot commented Apr 29, 2024

github-actions bot commented Apr 29, 2024

metalix2 left a comment

		"@skyscanner/bpk-foundations-web": "^17.11.0",
		"@skyscanner/bpk-svgs": "^19.3.0",

[LOOM-1377][multiple-components]: remove unrequired ts ignores and address any typing issues #3392

[LOOM-1377][multiple-components]: remove unrequired ts ignores and address any typing issues #3392

Conversation

mungodewar commented Apr 26, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Apr 26, 2024 • edited

Browser support

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

github-actions bot commented Apr 29, 2024

olliecurtis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 29, 2024

github-actions bot commented Apr 29, 2024

metalix2 left a comment

Choose a reason for hiding this comment

mungodewar commented Apr 26, 2024 •

edited

github-actions bot commented Apr 26, 2024 •

edited