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

Update fastdom #23076

Merged
merged 8 commits into from
Oct 8, 2020
Merged

Update fastdom #23076

merged 8 commits into from
Oct 8, 2020

Conversation

sndrs
Copy link
Member

@sndrs sndrs commented Oct 6, 2020

What does this change?

  • updates fastdom to latest version (0.8.5 to 1.0.9)
  • uses fastdom's new native promisifying for fastdom-promisify
  • fixes some seemingly unrelated tests that now appeared broken for me. i'm not sure how they were passing before...
    • projects/common/modules/identity/modules/switch.spec.js
    • projects/commercial/modules/hosted/onward-journey-carousel.spec.js

Why?

was looking to move fastdom-promisify to libs and/or use fastdom as a peerDep in commercial-core, and as i went to install it I realised the entire api had changed since we last updated it.

we're missing out on fixes (and types) and it would be valuable to be able to declare it as an up-to-date peerDep soon

Does this change need to be reproduced in dotcom-rendering ?

  • No

@sndrs sndrs requested review from gtrufitt and liywjl October 6, 2020 15:09
@sndrs sndrs requested a review from a team as a code owner October 6, 2020 15:09
@PRBuilds
Copy link

PRBuilds commented Oct 6, 2020

PRbuilds results:

Screenshots
wide.pngdesktop.pngtablet.pngmobile.png

💚 A11y validation
a11y-report.txt

💚 Microdata Validation
microdata.txt

Apache Benchmark Load Testing
loadtesting.txt

LightHouse Reporting
1601997384.report.html

--automated message

Copy link
Contributor

@mxdvl mxdvl left a comment

Choose a reason for hiding this comment

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

Good and chunky 🐘, but maybe the API update and the other changes should be split into different PRs?

const transform = (1 - expectedPage) * 100;

expect(
(document.querySelector(
'.js-carousel-pages'
): any).getAttribute('style')
).toEqual(
`-webkit-transform: translate(${transform || '-000'}%, 0);`
Copy link
Contributor

Choose a reason for hiding this comment

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

For another PR!

@@ -11,7 +11,7 @@ beforeEach(() => {
});

test('gets info', () => {
const el = $('.originalClassName');
const el = $('.originalClassName').get(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

i would but then the tests won't pass!

@@ -57,7 +57,7 @@ describe('Container Toggle', () => {
const toggle = new ContainerToggle(container);
toggle.addToggle();

fastdom.defer(1, () => {
fastdom.mutate(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -8,7 +8,7 @@ Before | After

## Fastdom

**Do not use fastdom** when inserting elements with steady page. The utility uses fastdom under the hood to read and write where required. It batches the insertion of elements, so that if multiple elements are queued in the same animation frame they will be inserted in the same fastdom.write and the position scrolled once to prevent excessive page jumping.
**Do not use fastdom** when inserting elements with steady page. The utility uses fastdom under the hood to read and write where required. It batches the insertion of elements, so that if multiple elements are queued in the same animation frame they will be inserted in the same fastdom.mutate and the position scrolled once to prevent excessive page jumping.
Copy link
Contributor

Choose a reason for hiding this comment

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

The steadypage utility was on for about half an hour and then we realised it was causing problems so I meant to fix it and that was like 4 years ago. 😆 (Not saying it's anything in this PR but probably something I can remove!)

Copy link
Member Author

Choose a reason for hiding this comment

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

alpha4life

Copy link
Contributor

@gtrufitt gtrufitt left a comment

Choose a reason for hiding this comment

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

Bit hard to review I guess, but looks good to me, +1 on upgrading it!

@sndrs
Copy link
Member Author

sndrs commented Oct 7, 2020

Bit hard to review I guess

i know – thank you! i think there must be a better way of managing these kind of small-but-sweeping changes. think a few may be on the way with commercial stuff...

@sndrs sndrs merged commit dd9a05b into main Oct 8, 2020
@sndrs sndrs deleted the update-fastdom branch October 8, 2020 11:00
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @sndrs 20 minutes and 56 seconds ago)

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.

5 participants