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

RNA: Add Jetpack footer #20287

Merged
merged 23 commits into from
Jul 12, 2021
Merged

RNA: Add Jetpack footer #20287

merged 23 commits into from
Jul 12, 2021

Conversation

kangzj
Copy link
Contributor

@kangzj kangzj commented Jul 7, 2021

Changes proposed in this Pull Request:

Added two components,

  1. A8C SVG Title: 'An Automattic Airline'
  2. Jetpack Admin footer, uses the SVG title above and a Jetpack branding
  3. Added plugin:@wordpress/eslint-plugin/i18n to check i18n text domain.

Jetpack product discussion

n/a

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

no

Testing instructions:

  • Go to projects/js-packages/components and run pnpm run test

  • Ensure unit tests pass

  • Checkout Search: Add search admin page footer #20252 and follow test instructions

  • Ensure components in footer are rendered properly

  • Ensure eslint fails if i18n text domain is set to strings other than jetpack

image

@kangzj kangzj added [Status] In Progress Admin Page React-powered dashboard under the Jetpack menu RNA [JS Package] Components labels Jul 7, 2021
@kangzj kangzj self-assigned this Jul 7, 2021
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello kangzj! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D63771-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
Copy link
Contributor

github-actions bot commented Jul 7, 2021

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.


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.

@kangzj kangzj force-pushed the add/rna-jetpack-footer branch from bf73aa2 to 3868d29 Compare July 7, 2021 07:21
@kangzj kangzj requested review from sdixon194, sergeymitr and a team July 7, 2021 07:27
@kangzj kangzj added this to the jetpack/10.0 milestone Jul 7, 2021
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Team Review labels Jul 7, 2021
Copy link
Contributor

@jsnmoon jsnmoon left a comment

Choose a reason for hiding this comment

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

Looks like this needs a rebase/master merge; diff currently includes changes from #20288.

@kangzj kangzj force-pushed the add/rna-jetpack-footer branch from 3868d29 to 05bed62 Compare July 8, 2021 00:50
@kangzj
Copy link
Contributor Author

kangzj commented Jul 8, 2021

Looks like this needs a rebase/master merge; diff currently includes changes from #20288.

Thank you @jsnmoon and @jeherve for the review 👍 . Rebased and addressed feedback.

@kangzj kangzj added [Status] Needs Team Review 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 Jul 8, 2021
@kangzj kangzj requested a review from jsnmoon July 8, 2021 01:40
Copy link
Contributor

@jsnmoon jsnmoon left a comment

Choose a reason for hiding this comment

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

Found places for small tweaks here and there. Nonetheless, this looks ready for crew review!

@jsnmoon jsnmoon added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Jul 8, 2021
@kangzj kangzj force-pushed the add/rna-jetpack-footer branch from 8328cd0 to 48c4164 Compare July 12, 2021 10:01
@kangzj
Copy link
Contributor Author

kangzj commented Jul 12, 2021

This looks good to me. I have one question here.

Good point. Committed. And also update #20252 for testing. Thanks 👍

@kangzj kangzj added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Jul 12, 2021
@kangzj kangzj requested a review from jeherve July 12, 2021 10:14
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me, although I can't seem to see the footer at all when using the Beta plugin; I only see it on my local installation. I think it may be that the components do not get built when using the Beta plugin?

In any case, that's probably not something to address in this PR. Logged this in #20330.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jul 12, 2021
@kangzj kangzj dismissed jsnmoon’s stale review July 12, 2021 22:00

feedback already addressed.

@kangzj kangzj merged commit ca0ab25 into master Jul 12, 2021
@kangzj kangzj deleted the add/rna-jetpack-footer branch July 12, 2021 22:02
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 12, 2021
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D63771-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@kangzj
Copy link
Contributor Author

kangzj commented Jul 12, 2021

🚢 r228524-wpcom

@sergeymitr
Copy link
Contributor

Hi @kangzj.
Good job on building the component.
We're about to build a similar one for header, and there'll likely be other layout components later on.

So I'm thinking that instead of putting them all into jetpack-components, we should group them all in the new package jetpack-layout.

Do you see any potential undesired consequences of doing that?
I didn't manage to find any consumers of the footer component, are there any?

@kangzj
Copy link
Contributor Author

kangzj commented Jul 14, 2021

So I'm thinking that instead of putting them all into jetpack-components, we should group them all in the new package jetpack-layout.

Do you see any potential undesired consequences of doing that?
I didn't manage to find any consumers of the footer component, are there any?

Hi @sergeymitr sure thing, I think it will be clearer. Tbh we are super happy just to consume them :-)

@kangzj
Copy link
Contributor Author

kangzj commented Jul 15, 2021

Hi @sergeymitr

Would it make sense to use the WordPress abstraction from @wordpress/element instead of React itself here? --@jeherve

Just while you are here, are we going to import @wordpress/element instead of React?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu [JS Package] Components RNA Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants