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

Newsletter Dashboard Widget: add footer #100040

Draft
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

holdercp
Copy link
Contributor

@holdercp holdercp commented Feb 19, 2025

Related to Automattic/loop#451

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@holdercp holdercp self-assigned this Feb 19, 2025
@matticbot
Copy link
Contributor

matticbot commented Feb 19, 2025

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • newsletter

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/newsletter-widget-footer on your sandbox.

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@holdercp
Copy link
Contributor Author

holdercp commented Feb 19, 2025

@arcangelini @allilevine With @spsiddarthan being off at a WC, would you mind looking over this PR and its related Jetpack one?

I'm looking for feedback on the approach given I'm not quite sure how to wrangle these links the "correct" way. The context is in https://github.com/Automattic/loop/issues/451 but I essentially need to link out to various pages from this wp-admin widget.

Where I'm headed

  • Because this widget could be used on a simple or Jetpack site (anywhere else?), these links change based on the env. For example, subscriber management on simple is at wordpress.com/subcribers/{hostname} and on a Jetpack site it's at cloud.jetpack.com/subscribers/{hostname}.
  • This led me down the path of determining what env the JS is rendering in and then conditionally rendering the correct links.
  • Does this seem right? Is there a better way to do this? I'm totally new to working with Jetpack/wp-admin so let me know if I'm missing something obvious.
  • Is there a better way to route other than building URL's? It seems odd to have to link out to Jetpack Cloud for somethings and for other things I use a relative URL because it should be based on the admin path.

This PR is just in a stubbed out state so ignore the messiness but I'd love some feedback on how I'm thinking about this.

@arcangelini
Copy link
Contributor

arcangelini commented Feb 20, 2025

@holdercp I think the idea from here would be to build the app itself inside a package in calypso. This way it can be used in calypso /home area as well as in the /wp-admin section. If we don't really care about the widget existing in Calypso at all then we could probably save ourselves some work and just put it all in Jetpack.

Is there a better way to route other than building URL's? It seems odd to have to link out to Jetpack Cloud for somethings and for other things I use a relative URL because it should be based on the admin path.

I believe all of this exists in Calypso as well (at least from my testing) so I would be happy to just default it to there unless there is an important reason for using Jetpack cloud

This led me down the path of determining what env the JS is rendering in and then conditionally rendering the correct links.

When I have done this in the past we have used wp_add_inline_script from the PHP entry point (in Jetpack) to pass in an object with helpful things like this. Example Exactly like you did : ) haha sorry I didn't look at the Jetpack PR fully before writing this.

@holdercp
Copy link
Contributor Author

@arcangelini thanks for the feedback.

I think the idea from here would be to build the app itself inside a package in calypso.

That's what I was gathering from this post: pdDR7T-1Gn-p2 But looking at the project thread it didn't seem like we were intending to use this in Calypso, so I started building things in the app. I'll get this confirmed in the PT though. There might not be too much extra work just putting it in a package and then we have the flexibility to render it in Calypso.

I believe all of this exists in Calypso as well (at least from my testing) so I would be happy to just default it to there unless there is an important reason for using Jetpack cloud

Do you mean all of these views that we're linking to here exist in calypso? I think I'm confused about routing in general, especially across Calypso, wp-admin, and Jetpack.

  • For example, to "Publish your next post" my assumption would be to link to "All Posts", which I know lives at wp-admin/edit.php and that doesn't change whether I'm on Simple or Atomic. But is there a Calypso view for this?
  • There's also "View your subscriber stats", which seems to exist in Calypso at /subscribers/{site} and as a Jetpack view at /wp-admin/admin.php?page=stats#!/stats/subscribers/{site} Ok, I didn't realize a link like /subscribers/{atomic site} worked. I think I'm misunderstanding how atomic sites are connected back to wpcom.

So it seems like we can use the wordpress.com/{earn|subscribers|etc}/{site} routes for these. Is that what you meant by these being in Calypso?

@holdercp holdercp force-pushed the add/newsletter-widget-footer branch from 9032416 to 3a4a650 Compare February 21, 2025 22:38
@holdercp holdercp changed the title Add/newsletter-widget-footer Newsletter Dashboard Widget: add footer Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants