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

Answers -> Search 2: headless unique customer components #113

Merged
merged 18 commits into from
Jul 12, 2022

Conversation

ElemelonWind
Copy link
Contributor

@ElemelonWind ElemelonWind commented Jul 5, 2022

Renamed all new customer-facing components, updated package.json. Part 3 will be updating imports/exports from updated search-core

J=SLAP-2204
TEST=auto

@ElemelonWind ElemelonWind marked this pull request as ready for review July 11, 2022 15:24
@ElemelonWind ElemelonWind requested a review from a team as a code owner July 11, 2022 15:24
@ElemelonWind ElemelonWind changed the title Customer facing renaming Answers -> Search 1: Customer facing renaming Jul 11, 2022
Copy link
Collaborator

@yen-tt yen-tt left a comment

Choose a reason for hiding this comment

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

we should point this to develop, or a feature branch like how Alex did for search-core!

src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
/**
* Common utility functions for manipulating Answers-related data.
*/
public readonly utilities = answersUtilities;
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets rename to searchUtilities? (ignore this if you are planning on changing this in part 2..)

src/search-headless.ts Outdated Show resolved Hide resolved
etc/search-headless.api.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@ElemelonWind ElemelonWind changed the base branch from main to develop July 11, 2022 16:56
@coveralls
Copy link

coveralls commented Jul 11, 2022

Pull Request Test Coverage Report for Build 2652011560

Details

  • 4 of 7 (57.14%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 92.111%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/deprecated.ts 0 3 0.0%
Totals Coverage Status
Change from base Build 2651690679: -0.6%
Covered Lines: 325
Relevant Lines: 346

💛 - Coveralls

@oshi97
Copy link
Contributor

oshi97 commented Jul 11, 2022

technically the imports/exports from updated search-core are also consumer facing, I would update the PR title!

@oshi97
Copy link
Contributor

oshi97 commented Jul 11, 2022

I told this to Alex too - could we move the package.json changes to their own PR? Otherwise it's difficult to review a PR this large - github really struggles here. Changing the package.json package name also changes all of the doc links so that's why literally every doc file changes

@ElemelonWind ElemelonWind changed the title Answers -> Search 1: Customer facing renaming Answers -> Search 1: headless unique customer components, package.json Jul 11, 2022
@ElemelonWind
Copy link
Contributor Author

I told this to Alex too - could we move the package.json changes to their own PR? Otherwise it's difficult to review a PR this large - github really struggles here. Changing the package.json package name also changes all of the doc links so that's why literally every doc file changes

Is there an easy way to split it into a new PR?

@oshi97
Copy link
Contributor

oshi97 commented Jul 11, 2022

I would still keep this PR/branch - after the package.json PR is merged in you can do a rebase with a theirs strategy (see: https://stackoverflow.com/questions/16825849/choose-git-merge-strategy-for-specific-files-ours-mine-theirs) or however you would like to merge things back up

@oshi97
Copy link
Contributor

oshi97 commented Jul 11, 2022

I told this to Alex too - could we move the package.json changes to their own PR? Otherwise it's difficult to review a PR this large - github really struggles here. Changing the package.json package name also changes all of the doc links so that's why literally every doc file changes

Is there an easy way to split it into a new PR?

You can make a new branch, and git cherry-pick the changes you want into that new branch. I like to use sublime merge for this. You could also just copy paste the new package.json file since that should be the only change.
After that run npm install and the doc generation again

@ElemelonWind ElemelonWind changed the title Answers -> Search 1: headless unique customer components, package.json Answers -> Search 2: headless unique customer components Jul 11, 2022
@ElemelonWind
Copy link
Contributor Author

I told this to Alex too - could we move the package.json changes to their own PR? Otherwise it's difficult to review a PR this large - github really struggles here. Changing the package.json package name also changes all of the doc links so that's why literally every doc file changes

Is there an easy way to split it into a new PR?

You can make a new branch, and git cherry-pick the changes you want into that new branch. I like to use sublime merge for this. You could also just copy paste the new package.json file since that should be the only change. After that run npm install and the doc generation again

ok! new PR here: #115

@ElemelonWind ElemelonWind changed the base branch from develop to feature/answers-to-search July 11, 2022 18:42
src/index.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,29 @@
import { searchUtilities, provideHeadless } from ".";
Copy link
Collaborator

@yen-tt yen-tt Jul 11, 2022

Choose a reason for hiding this comment

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

eslint warnings, single quotes for import statements. (should add eslint github action to this repo...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were some other lint warnings like dual exports or something which is probably why it's not a github action :)

*
* @deprecated answersUtilities has been deprecated and renamed to searchUtilities
*/
export const answersUtilities = searchUtilities;
Copy link
Collaborator

@yen-tt yen-tt Jul 11, 2022

Choose a reason for hiding this comment

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

remember to import these stuff into index.ts and export them there. Otherwise, user wouldn't have access to these deprecated stuff

*
* @deprecated provideAnswersHeadless has been deprecated and renamed to provideHeadless
*/
export const provideAnswersHeadless = provideHeadless;
Copy link
Collaborator

Choose a reason for hiding this comment

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

eslint warnings, extra space before export in this file, and in the line below here

@@ -1,7 +1,7 @@
import { Matcher, QuerySource, QueryTrigger } from '@yext/answers-core';
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename this file?

@oshi97
Copy link
Contributor

oshi97 commented Jul 12, 2022

could you remove the tests/.DS_Store file and also add it to the gitignore (I think you can just add a line with .DS_Store?)

@ElemelonWind
Copy link
Contributor Author

could you remove the tests/.DS_Store file and also add it to the gitignore (I think you can just add a line with .DS_Store?)

lmao I can't figure out how to remove it - the delete button is grayed out and it's not in VSCode even after I pull (?)

I added to gitignore though

.gitignore Outdated
temp/
.DS_STORE
Copy link
Contributor

Choose a reason for hiding this comment

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

the casing is off here! it's .DS_Store

/**
* Common utility functions for manipulating Answers-related data.
* Common utility functions for manipulating Search-related data.
*/
public readonly utilities = answersUtilities;
Copy link
Contributor

Choose a reason for hiding this comment

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

bumping yen's comment to rename to searchUtilities!

@@ -66,7 +66,7 @@ const mockedCore: any = {
filterSearch: jest.fn(() => Promise.resolve({}))
};

const answers = new AnswersHeadless(mockedCore, mockedStateManager, new HttpManager());
const answers = new SearchHeadless(mockedCore, mockedStateManager, new HttpManager());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this variable too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's like a million variables named answers so Tom said we'd rename those in a later phase!

Copy link
Contributor

Choose a reason for hiding this comment

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

ooh gotcha!

@@ -19,7 +19,7 @@ export interface QueryState {
*/
queryTrigger?: QueryTrigger,
/**
* The source of the query (from a standard Answers integration, an Answers
* The source of the query (from a standard Search integration, an Search
Copy link
Contributor

@oshi97 oshi97 Jul 12, 2022

Choose a reason for hiding this comment

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

an -> a

@@ -3,7 +3,7 @@ import { State } from './state';
import StateListener from './state-listener';

/**
* Manages the information contained in the state for an AnswersHeadless instance.
* Manages the information contained in the state for an SearchHeadless instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

an -> a

Copy link
Contributor

@oshi97 oshi97 left a comment

Choose a reason for hiding this comment

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

lgtm!

@ElemelonWind ElemelonWind merged commit da871b1 into feature/answers-to-search Jul 12, 2022
@ElemelonWind ElemelonWind deleted the rename-search branch July 12, 2022 16:51
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.

None yet

5 participants