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

Gallery block: Refactor "Settings" panel to use Toolspanel #67957

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

Mayank-Tripathi32
Copy link
Contributor

part of #67813
resolves #67893

Before

image

After

image
image

Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mayank-Tripathi32 Mayank-Tripathi32 changed the title feat: migrate to ToolsPanel Gallery block: Refactor "Settings" panel to use Toolspanel Dec 13, 2024
@Mayank-Tripathi32
Copy link
Contributor Author

Working on fixing unit test

@fabiankaegy
Copy link
Member

fabiankaegy commented Dec 13, 2024

@Mayank-Tripathi32 looks like there is already a PR for this one #67904

But that one also has the same issue with the unit tests. So feel free to collaborate in case you already have a solution for the tests :)

@Mayank-Tripathi32
Copy link
Contributor Author

@Mayank-Tripathi32 looks like there is already a PR for this one #67904

But that one also has the same issue with the unit tests. So feel free to collaborate in case you already have a solution for the tests :)

I tried to figure it out, but most issues occur on mobile, and I'm unsure how to fix them. If you have any resources, I'd be happy to try again.

@fabiankaegy fabiankaegy added [Type] Enhancement A suggestion for improvement. [Block] Gallery Affects the Gallery Block - used to display groups of images labels Dec 13, 2024
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I think the unit test failure is based on the fact that ToolsPanel and ToolsPanelItem are not available in the React Native version. There are two solutions:

  1. Split the Edit component into a web version and a React native version. That is, create a edit.native.js file for the React Native version. Leave the Edit component unchanged in the React native version.
  2. Render the component conditionally using Platform.isWeb or Platform.isNative.

I think the latter approach is better for the Gallery block, since the Platform is already used for block controls etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor "Settings" panel of Gallery block to use ToolsPanel instead of PanelBody
3 participants