Size Change: -244 B (0%)

Total Size: 1.15 MB

_{compressed-size-action}

Hey @ttahmouch 👋, I see you requested a review for this PR but looks like it's still in draft, just wanted to check whether is ready or if you're looking for something specific to get reviewed before marking it as ready for review 🤔 .

Hey @ttahmouch 👋, I see you requested a review for this PR but looks like it's still in draft, just wanted to check whether is ready or if you're looking for something specific to get reviewed before marking it as ready for review 🤔 .

Hey, @fluiddot . I marked it as Draft because I still had intended to make some minor refactors, but I ended up dealing with IDE issue with ESLint for a little while yesterday afternoon. Apologies.

I may remove the mocked hook in test/unit/config/global-mocks.js since that I don't believe it's necessary with the other mock in test/native/setup.js. I also planned to push changes making the default style values for active icons feel less arbitrary in packages/block-library/src/social-link/edit.native.js. I was going to also consider if there's any useful utils available or that I could make to simplify staging the tests, but I'm conflicted because I prefer WET instead of DRY test implementations to make them easier to understand.

I just noticed some test failures unrelated to the tests in this PR because the snapshot assertions they are running were affected by the default active icon styles I added in the <EditSocialLink/> component. I'll try to ensure that the snapshots are updated correctly soon.

https://github.com/WordPress/gutenberg/runs/5324097673?check_suite_focus=true

@ttahmouch The changes look good to me. Your reasoning to keep avoid going DRY at this point also makes sense to me.
I also appreciate that you are keeping this PR small. Much easier to grok the test flow.

I ran the tests locally and they all passed. I did get this ugly warning :

Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

but it didn't break anything. I'm guessing I have something off on my dev environment.

@ttahmouch The changes look good to me. Your reasoning to keep avoid going DRY at this point also makes sense to me. I also appreciate that you are keeping this PR small. Much easier to grok the test flow.

❤️ 🙏 Thank you so much for taking a look, @jhnstn . I appreciate it a lot. I'm glad you feel the same way regarding DRY.

I ran the tests locally and they all passed. I did get this ugly warning :

Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

but it didn't break anything. I'm guessing I have something off on my dev environment.

I did get that ugly warning as well, but it didn't impact the test assertions since I saw them go from red to green when TDDing it. I thought about spending some time trying to fix whatever was wrong with the forwardRef in order to make the warning go away, but it didn't feel like something that needed to be included in this PR at the moment. Does that make sense?

I did get that ugly warning as well, but it didn't impact the test assertions since I saw them go from red to green when TDDing it. I thought about spending some time trying to fix whatever was wrong with the forwardRef in order to make the warning go away, but it didn't feel like something that needed to be included in this PR at the moment. Does that make sense?

Agreed. Doesn't seem to block this work so no need to fix it here.

All of the tests and workflows are ✅ now, @jhnstn . If you have some time soon, would you mind giving the remaining changes a cursory glance? They're fairly small.

The latest changes look good. Getting the snapshots sorted out does feel like a priority but your solution is the right choice for this effort

I agree, and I appreciate your input and the review. It means a lot to me. I just wanted to time box the changes for this specific PR, and addressing the snapshot assertions was taking longer than I thought was reasonable at the moment. This just felt like a good compromise.

Yay, cool to see more automated tests merged! @ttahmouch , for easier reference, can you call out in the PR description exactly which tests (titles) have been added by the PR? And more interestingly, can we drop those from the manual testsuites or there are parts that are performed in manual but not yet in the automated tests so we need to keep those manual for now?