Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Blocks: Show Blocks if they're missing a plan, add Upgrade Nudge #12823
Blocks: Show Blocks if they're missing a plan, add Upgrade Nudge #12823
Changes from 34 commits
d81f1b9
99976e9
a5ed423
cf5587d
ae97d61
24a131c
ea06449
21ec9fa
3ab6901
03bb5e5
f721591
993bc53
9894bd6
cde52f8
b0ce0be
157c759
7374dc0
f221354
f0ab4eb
7cc31d5
7cd8cd1
ee87825
4ec054f
c802926
10d28ca
b0c395f
c64d6d5
9ed7fe9
c871c99
9669ec3
ae89afc
07bf211
1157ae6
4d6617b
e7fdbb2
69d50cd
73810d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But not
isPrimary
, per paAmJe-ll-p2#comment-741There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to leave
wordpress-com
here but just noting that Publicize usesjetpack/
prefix for its data store — better for consistency?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, TBH. I was using the prefix to indicate the data source -- which is the WordPress.com REST API, not the self-hosted Jetpack site's 🤷♂️
As in, the store name represents the data source -- if we were fetching other data from WP.com, I'd probably call the stores accordingly, e.g.
wordpress-com/themes
etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That reasoning makes sense. 👍 Just wasn't clear so might need documenting in the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda make this up as I go 😂 There isn't a lot of precedent for
@wordpress/data
-type stores in Gutenpack (there are a few of one-off API fetches outside of stores, though), so it seems kind of a specific information to put into docs. I can try to generalize it so it's not more confusing than helpful (because of its specificity) 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally fine to make it up as we go but once we set a pattern for others to follow, it should be documented unless it's obvious by looking at the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, I think in this case I'd call it
jetpack/plans
still.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what made
jetpack/plans
sound wrong to me is that this code is also synced to WP.com, and it'll get plans information solely for WP.com plans there -- so thejetpack/
prefix seemed kinda misleading for that.The inverse, however -- having the
wordpress-com/
prefix in Jetpack code -- seemed acceptable, since WP.com is indeed the source of information here.However, if you're feeling strongly about this, I can change the prefix to
jetpack/
:)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtias can you please elaborate why's for your opinion, thanks. :-D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plans section in WP.com also has the Jetpack logo, so it's not that odd to me :)