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

Remove location api #1569

Merged
merged 4 commits into from
Nov 28, 2023
Merged

Remove location api #1569

merged 4 commits into from
Nov 28, 2023

Conversation

oluwatimio
Copy link
Contributor

@oluwatimio oluwatimio commented Nov 28, 2023

Background

Fixes https://github.com/Shopify/core-issues/issues/62603

Solution

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@oluwatimio oluwatimio self-assigned this Nov 28, 2023

This comment has been minimized.

Copy link
Member

@robin-drexler robin-drexler left a comment

Choose a reason for hiding this comment

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

type checks are complaining, but it's only an unused import

location: StatefulRemoteSubscribable<{
pathname: string;
search: string;
}>;
Copy link
Contributor

@brianshen1990 brianshen1990 Nov 28, 2023

Choose a reason for hiding this comment

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

also here? we should be able to remove the omit

export interface Docs_FullPageApi extends Omit<FullPageApi, 'location'> {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yes, Id also have to update shopify.dev 😬 alongside this since that references the types

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be OK, we are using Docs_FullPageApi instead of FullPageApi if I remember correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no wouldnt need to do that since we omit location

@oluwatimio oluwatimio merged commit cbc51b3 into unstable Nov 28, 2023
5 checks passed
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