[RNMobile] Tiled Gallery Block: Use default colour for block icon

[SiobhyB]

Fixes wordpress-mobile/gutenberg-mobile#4158

Changes proposed in this Pull Request:

• The fill parameter is removed from the icon's Path element. The fact that this parameter was setting the icon's fill to currentColor was causing it to appear as the wrong colour in the apps. By removing this parameter, the icon now reverts back to the default colour that all other blocks use for their icons.

Mobile Gutenberg PR: wordpress-mobile/gutenberg-mobile#4193

Jetpack product discussion

Main project thread for the Tiled Gallery block on mobile: p9ugOq-1Tb-p2

Does this pull request change what data or activity we track or use?

No, it doesn't.

Testing instructions:

To test, it should be verified that:

1. The icon colour now matches up with other icons within the Android and iOS app.
2. The icon colour remains unaffected on the web.

Apps

• With these branches checked out and the Gutenberg Mobile demo app running, open the block inserter menu by tapping the + icon in the editor's toolbar.
• Verify that the icon colour for the Tiled Gallery block matches up with the icons for the other blocks.

Before	After

Web

• Navigate to the editor on a Jetpack-connected site (not a WordPress.com site) and add the Tiled Gallery block.
• Verify that the icon appears green within the block inserter menu and the default dark grey/black within the empty block.

Screen.Recording.2021-11-03.at.22.10.59.mov

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello SiobhyB! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D69460-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

[github-actions]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

• ✅ Include a description of your PR changes.
  
• ✅ All commits were linted before commit.
  
• ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  
• ✅ Add testing instructions.
  
• ✅ Specify whether this PR includes any changes to data or privacy.
  
• 🔴 Add changelog entries to affected projects
  

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

---

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

---

🔴 Action required: Please add missing changelog entries for the following projects: projects/plugins/jetpack

Use the Jetpack CLI tool to generate changelog entries by running the following command: jetpack changelog add.
Guidelines: /docs/writing-a-good-changelog-entry.md

---

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

---

Jetpack plugin:

• Next scheduled release: December 7, 2021.
• Scheduled code freeze: November 30, 2021.

[SiobhyB]

👋 @kraftbj, you were a recommended reviewer for this file and I'd be grateful if I could get your eyes on it from a web perspective.

I don't believe removing the fill="currentColor" line, as suggested in this PR, will have any impact on the web. I was able to verify this on a non-WordPress.com site via the testing shown in the PR description. I haven't yet been able to test this on a WordPress.com site as I don't currently have a sandbox. My read of the code makes me suspect the block's icon will pick up the correct colours on WordPress.com. I could be wrong with this and/or overlooking something.

I'd be grateful to hear if you have any concerns about how removing this line of code could impact the web. If so, we could then create a native-specific version of the index.js file for this change instead. 🙇‍♀️

[SiobhyB]

👋 @guarani, @illusaen, as part of this PR I'd be interested to hear your thoughts on whether we should create a native-specific version of the tiled-gallery/index.js file for the changes we need.

The change in this PR doesn't impact the web in my testing (though I've yet to test on WordPress.com, as I'm pending sandbox approval, I've only been able to test on self-hosted at the time of writing). As such, it didn't feel like the change warranted a separate file and I went for sticking to the index.js file, to avoid potential confusion for developers in the future. I do have some concern I may be missing something, though. What are your thoughts?

[guarani]

👋 Hi @SiobhyB, I don't think there's a clear answer here. I think we need to look into why this change was necessary and expand on how this was tested.

• Is the removal of fill="currentColor" the right solution for mobile and web? What role did fill play on web? Why did fill work on web but not on mobile? The PR description touches on this, but I think could go deeper.

• Could you expand on how this was tested for regressions on the web? You mention self-hosted, would that be via a local Docker environment?

• This isn't much information on regression testing that I could find, the closest would be this doc which states:

  Test using Core’s block editor and latest Gutenberg plugin.

  That might be what you did, but I wanted to share just in case it's useful.

[illusaen]

LGTM!

[illusaen]

So looking into the other icons Jetpack uses, it seems like they're using renderMaterialIcon function. IMO we should use that so we can be in line with them.

[SiobhyB]

@guarani, thanks for these questions!

Is the removal of fill="currentColor" the right solution for mobile and web? What role did fill play on web? Why did fill work on web but not on mobile? The PR description touches on this, but I think could go deeper.

I'm not clear what purpose the line was serving on web and wasn't able to find a clear answer through git blame or similar. It doesn't appear to actually be doing anything, but it could be the case I'm missing something.

I took a look at other blocks, and there isn't a clear, consistent usage of this attribute that I could find.

These blocks use fill="none":

• Simple Payments
• Subscriptions

These blocks don't use a fill attribute:

• Eventbrite
• MailChimp
• Pinterest
• Recurring Payments

Could you expand on how this was tested for regressions on the web? You mention self-hosted, would that be via a local Docker environment?

Yes, a local Docker environment is exactly what I meant. :)

I also need to test on WordPress.com, given that the icon colours are not green for Jetpack blocks on WordPress.com, in contrast to self-hosted. I've requested a sandbox so that I can test further.

[guarani]

I also need to test on WordPress.com, given that the icon colours are not green for Jetpack blocks on WordPress.com, in contrast to self-hosted. I've requested a sandbox so that I can test further.

Thanks for sharing! I wasn't aware of this color difference between WP.com and Jetpack-connected-self-hosted. I see now that logic is found here. It also looks like fill="currentColor" means: use the parent HTML element's color for the icon. I think the parent color may be defined here and applied here, so it gets used by the SVG icon.

Regarding other blocks:

These blocks use fill="none":

These blocks, "Simple Payments" and "Subscriptions" seem to be only available on WP.com, which may explain why they don't need to inherit a color (they only need to have black icons, never green).

These blocks don't use a fill attribute:

That's a good find. It looks like even though a fill isn't provided on the SVG itself, fill: currentColor is applied anyway via CSS here on the web. That could explain why it seems no longer necessary to set fill: currentColor on the SVG: it's already set via CSS.
On mobile however, the logic is a bit different. I'm not certain, but I think the use of fill was making the icon blue-ish on mobile's block picker because it was inheriting a color from a parent element (but I'm not sure which). It's worth noting that the color seemed correct (dark gray) in the slash inserter, so there it might have inherited the right color by chance.

---

I feel like this PR implements the correct approach. I don't think it's necessary to split index.js into web and mobile specific files. Your test on WP.com should be enough to ensure this works.

If you prefer, it might be worthwhile to make this PR target master instead. That way we can get input from web folks now rather than having to remember to ask web folks to review this particular change when merging the feature branch to master later. If so, after merging to master we'd have to update our feature branch to bring the change in. This is totally up to you.

[guarani]

So looking into the other icons Jetpack uses, it seems like they're using renderMaterialIcon function. IMO we should use that so we can be in line with them.

It looks like this function adds another <Path> component to the icon you pass in. I'm not sure what that's for though.

It also adds xmlns (not sure of its significance, maybe just a best practice) and sets a viewBox (again not sure how necessary this is, given our icons is 24px square and the default viewBox value is also 24px). It might simplify things though by allowing us to pass it the <Path> element without the surrounding <SVG> element, since all that is handled by renderMaterialIcon.

The base branch was changed.

[SiobhyB]

@guarani, wow, thank you for all that extra digging to uncover the reasoning for those differences! That all makes sense.

This has now been tested on WordPress.com, thanks to @jeherve's guidance, and it's confirmed there are no regressions. 🎉

I considered merging to master but, for the sake of simplicity, would like to opt to merge into the feature branch for now. When switching between branches the previous approvals were automatically dismissed. If you agree that merging directly to the feature branch makes sense for now, would you be able to approve again?

Thanks!

[SiobhyB]

Thank you @jeherve! 🙇‍♀️

[guarani]

I considered merging to master but, for the sake of simplicity, would like to opt to merge into the feature branch for now. When switching between branches the previous approvals were automatically dismissed. If you agree that merging directly to the feature branch makes sense for now, would you be able to approve again?

Sounds perfect 👍 Thanks for fixing this!