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.

Sign up for GitHub

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

Jump to bottom

Adding BottomSheet settings for Tiled Gallery #21028

Merged
merged 15 commits into from
Sep 23, 2021
Merged

Adding BottomSheet settings for Tiled Gallery #21028

merged 15 commits into from
Sep 23, 2021

Conversation

illusaen
Copy link
Contributor

@illusaen illusaen commented Sep 10, 2021 •
edited
Loading

Work for

Changes proposed in this Pull Request:

  • Adds settings (Rounded corners, columns, link to) to Tiled Gallery

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

My PR does not change any tracked data/activity.

Screenshots

Simulator.Screen.Recording.-.iPhone.12.-.2021-09-10.at.05.08.47.mp4

Testing instructions:

  • Add tiled gallery block
  • Click settings button
  • See that there are Columns, Rounded corners, and Link to settings.
  • Change setting for column
  • Dismiss settings
  • Reopen settings and see that settings are saved
  • Repeat for Rounded corners
  • Repeat for Link to

@illusaen illusaen requested a review from guarani September 10, 2021 10:17
@github-actions github-actions bot added [Block] Tiled Gallery [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Sep 10, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Sep 10, 2021 •
edited
Loading

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: October 5, 2021.
  • Scheduled code freeze: September 28, 2021.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Sep 10, 2021
@illusaen illusaen added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 10, 2021
@illusaen illusaen requested review from a team as code owners September 21, 2021 21:30
@guarani guarani changed the base branch from rnmobile/add/tiled-gallery-block-square-layout to rnmobile/add/tiled-gallery-block September 21, 2021 21:31
@guarani guarani removed request for a team September 21, 2021 21:31
@guarani
Copy link
Contributor

guarani commented Sep 21, 2021

Sorry about the extra reviewers I added here my mistake, I removed them now.

guarani
guarani reviewed Sep 21, 2021
View reviewed changes
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

👋 @illusaen, I've updated this PR (not without the minor hiccup above, sorry about that!).

It's looking good, just wanted to ask a couple of things:

  • in the video on the PR, at the 5s mark you can see the "Link to" row in the bottom sheet is missing left spacing, do you think this should be addressed now or later?
  • I saw some warnings about use of __ in projects/plugins/jetpack/extensions/blocks/tiled-gallery/settings.native.js. It seems like a second argument of jetpack is needed as a "text domain" (example here). Again do you think these should be fixed now or later?

@guarani
Copy link
Contributor

guarani commented Sep 22, 2021

I added a commit to fix the "Link to" row spacing, "text domain" warnings, and an unused import that was left over from my previous merge conflict resolution. Once all CI checks pass except for the ones we expect will fail, I'll go ahead and merge this to the feature branch.

@guarani guarani mentioned this pull request Sep 23, 2021
Merged
2 tasks
@guarani
Copy link
Contributor

guarani commented Sep 23, 2021

Summary of failing tests:

Tests / PHP tests: PHP 7.4 WP master

Based on this comment, this failure happens to other PRs and can be ignored if it happens.

Tests / PHP tests: PHP 7.4 WP previous

This is the first time I've seen this test fail on our PRs, but looking at currently open PRs at time of writing, this seems to be failing across the board. I think this can be ignored for now as it's not a required check.

Linting / Changelogger use

I expect this to fail. We'll be adding the changelog in a future PR to merge the feature branch to master (see this comment).

#### `codecov/patch``

The tests being added for the native mobile Tiled Gallery block currently live in the Gutenberg Mobile repo, which means they don't count towards code coverage (see here). That said, this particular change doesn't include new tests.

wpcom — Could not cleanly apply this PR to synced wpcom files

I got confirmation that this is fine to ignore for now since we're merging to a feature branch. When we merge to master, if this check fails again, we can address there.

@guarani guarani self-requested a review September 23, 2021 02:44
guarani
guarani approved these changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

LGTM, I've only added minor changes to this myself.

@guarani guarani merged commit 4c3ebd7 into rnmobile/add/tiled-gallery-block Sep 23, 2021
@guarani guarani deleted the tiled-gallery-settings branch September 23, 2021 02:50
SiobhyB pushed a commit that referenced this pull request Oct 28, 2021
@illusaen @guarani
Adding BottomSheet settings for Tiled Gallery (#21028)
c1ec7be 
Add BottomSheet settings

Co-authored-by: Wendy Chen <[email protected]>
Co-authored-by: Paul Von Schrottky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guarani guarani guarani approved these changes

Assignees
No one assigned
Labels
[Block] Tiled Gallery [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@illusaen @guarani @matticbot