-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: load favicon from WordPress server #119
Conversation
🦋 Changeset detectedLatest commit: 7fea521 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
LGTM
…ifferent size of Icon as favicon, 180x180 res icon as apple touch icon and 270x270 as msApplicationTileImage
…e files, move Icon types to the types package
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.
Functionally working 🚀
Feedback on composability/code quality patterns below:
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.
Functionally, things are passing, but I would like @ayushnirwal 's feedback on the composability (generally, and per the open conversations on this diff).
Once that's done (since the package diff might change) npm run changeset
and we should be able to merge this.
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.
Although the patterns around queries, documents, client management are not ideal in the current state. I'm willing to merge this PR (after the last few feedbacks are addressed) for the following reasons.
- It works.
- This adds parsers, some of the types around favicon and meta data management. When we implement the metadata management and data fetching management interfaces these will be used.
@ayushnirwal can you flag some parts of the diff that deserve a I don't want to merge this PR without making sure we have a (vague) plan to follow it up. |
import type { DocumentNode, TypedQueryDocumentNode } from 'graphql';
export type Variables = Record< string, unknown >;
export type FetchQuery< T = unknown, V extends Variables = Variables > = (
queryKey: string[],
document: DocumentNode | TypedQueryDocumentNode< T, V >
) => Promise< T >;
export type getServerClient = (
url: string,
options: Record< string, unknown >
) => ServerClient;
export type getBrowserClient = (
url: string,
options: Record< string, unknown >
) => BrowserClient;
export interface ServerClient {
fetchQuery: FetchQuery;
}
export interface BrowserClient {} These types/interfaces are the general broad strokes for the composability in our data-fetching mechanisms. This allows users to
|
@ayushnirwal anything specific to favicons / site metadata (see the remaining open conversations on this PR) or just the underlying Query API? |
|
@SH4LIN what's with the failing tests? Also can you add a changeset ? (Once those are resolved this should be good to merge) |
@justlevine Checks are passing now |
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.
Reminder, changesets is "dumb" and major|minor|patch
is mapped blindly to x.y.z
, not real semver
What
Related Issue(s):
How
Checklist
npm run changeset
.