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

Implement additional googlesitekit-site store selectors #1000

Closed
felixarntz opened this issue Dec 27, 2019 · 7 comments
Closed

Implement additional googlesitekit-site store selectors #1000

felixarntz opened this issue Dec 27, 2019 · 7 comments
Labels
P0 High priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented Dec 27, 2019

Feature Description

Following #999, the core/site store should include further selectors for data that is passed to JS via PHP. These changes should be added to the googlesitekit-site script.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The core/site store should have the following additional selectors (for data passed from PHP):
    • getReferenceSiteURL()
    • getHomeURL()
    • getAdminURL()
    • getAMPMode()
    • isAMP()
      • this is just for convenience, it should rely on the ampMode value from PHP, basically just !! ampMode
    • getCurrentReferenceURL()
      • this should be the frontend URL of the current resource, e.g. if you're editing a single post in the backend or viewing a single post in the frontend, it should be the post permalink; if you're on the blog homepage, it should be the blog page's URL
      • basically, it should come from Context::get_reference_canonical()
      • in the future we should have support for more things than singular posts and the blog home page
    • getCurrentEntityType()
      • this is based on the current reference URL, it is for now always post, or home, or nothing
    • getCurrentEntityTitle()
      • this is based on the current reference URL, it is for now either the post's title, or the title of the blog homepage, or nothing
    • getCurrentEntityID()
      • this is based on the current reference URL, it is for now the post's ID (of course only if it is a single post
      • in the future, this could also be a term ID, for term archives of a certain term, but that is not relevant for now
  • all these selectors should be sourced with data that is available on pageload via PHP; in other words, the script should receive a before_print callback in PHP that adds the data to some specific JS global (_googlesitekitSiteData), solely for consumption by that script
    • we already pass a lot of this information from PHP to existing scripts, but let's ignore that existing code and build this properly scoped to our new script

Implementation Brief

  • Add a before_print option to the asset entry in Assets.php created in Scaffold googlesitekit-site package and core/site store #999 that adds all relevant data from PHP for the selector to consume when registered. I'd actually like this global data to be deleted after the resolver (see below) so we don't leave globals around and accidentally use them 😄
  • Call a receiveSiteInfo() action to populate the data from the global data passed from PHP after any selector that relies on the data in receiveSiteInfo() is called. This can use the resolver pattern used in Gutenberg so that data is loaded only when needed. In this case the data will often be needed, but it's a good precedent to set for other selectors.
  • Add all data and selectors mentioned in the ACs, including tests for each.

Changelog entry

  • Add several additional selectors for commonly used site data to the core/site datastore.
@felixarntz
Copy link
Member Author

@tofumatt Regarding the setSiteInfo() action, can we make this so that it's automatically called whenever one of the consuming selectors is accessed for the first time (i.e. like the resolvers here)? I also like the pattern used in that Gutenberg code, maybe we can call it receiveSiteInfo()? That would indicate it's not actually an action outside parties should use to set data, it's only to "receive/refresh" data.

@felixarntz felixarntz assigned tofumatt and unassigned felixarntz Jan 17, 2020
@tofumatt
Copy link
Collaborator

Thanks for waiting for a response on this IB; finally getting back to it! 😅

To answer your question about using resolvers, we should be able to do that. I've updated the IB to mention we'll take that approach (and why).

For this particular set of selectors it isn't really needed as a lot of this data will be used for nearly every page, but I like the practice of using resolvers to load the data only once it's been requested, so let's set a good example and do it here! 👍

@felixarntz
Copy link
Member Author

IB ✅

@tofumatt Still, let's call the action receiveSiteInfo(), as it is not an action that should be publicly called, and this naming is a good distinction for that. set...() should only be used if we actually intend for other people to use this.

@tofumatt
Copy link
Collaborator

Oh, yes, totally! I meant to add that from our discussion and it slipped my mind :-)

I updated it in the IB 👍

@felixarntz
Copy link
Member Author

felixarntz commented Mar 4, 2020

@tofumatt Regarding the PHP output to pass the information to JS:

  • Wherever a relevant value is already part of _googlesitekitBaseData (e.g. the AMP information), that value should be used from there (i.e. googlesitekit-base-data should become a dependency of googlesitekit-datastore-site).
  • Whatever is not already in there, should become part of the new _googlesitekitDatastoreSiteData global, which should be registered in PHP as a Script_Data instance (and then also be added to the googlesitekit-datastore-site dependencies).

See #1189 for context.

@felixarntz
Copy link
Member Author

@tofumatt Would be great if you could prioritize this issue and #1224 (once that's fully defined) over the next days/week as #1101 (which @aaemnnosttv will work on) will require parts of both.

Let's make sure to fully stick to the selectors/actions naming outline we put together in both of the store issues, as the work on #1101 needs to already start in the meantime, assuming that those will be the selectors/actions available.

@felixarntz felixarntz modified the milestones: Sprint 18, Sprint 19 Mar 12, 2020
@tofumatt tofumatt assigned felixarntz and unassigned tofumatt Mar 24, 2020
@felixarntz felixarntz assigned tofumatt and aaemnnosttv and unassigned felixarntz Mar 25, 2020
@tofumatt tofumatt removed their assignment Mar 27, 2020
@eclarke1 eclarke1 modified the milestones: Sprint 19, Sprint 20 Mar 31, 2020
@felixarntz felixarntz added the QA: Eng Requires specialized QA by an engineer label Apr 1, 2020
@cole10up
Copy link

cole10up commented Apr 8, 2020

Ran regression around basic functionality. Installed and setup SK, connected all modules, confirmed functionality, disconnected modules, disconnected SK, deactivated and removed SK. Reinstalled and confirmed everything remained functional after setup a 2nd time.

Passed QA ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants