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.

Sign up for GitHub

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

Jump to bottom

chore(Angular IS): upgrade query-suggestions example to angular 12 #286

Merged
merged 4 commits into from
Oct 26, 2021
Merged

chore(Angular IS): upgrade query-suggestions example to angular 12 #286

merged 4 commits into from
Oct 26, 2021

Conversation

dhayab
Copy link
Member

@dhayab dhayab commented Oct 25, 2021

As specified in other PRs, the diff is a bit unreadable, but the process I followed was this one:

  • I created a new project from create-instantsearch-app with the Angular InstantSearch template
  • I copied the code example from the previous version and made the necessary adjustments

see #255

@dhayab dhayab force-pushed the angular-12-query-suggestions branch from cc3c590 to 7e055c0 Compare October 25, 2021 13:17
@dhayab dhayab requested review from a team, tkrugg and sarahdayan and removed request for a team October 25, 2021 13:34
@francoischalifour
Copy link
Member

The keyboard navigation doesn't seem to work on the preview.

@dhayab dhayab force-pushed the angular-12-query-suggestions branch from 7e055c0 to 69b30a7 Compare October 25, 2021 15:56
@dhayab
Copy link
Member Author

dhayab commented Oct 25, 2021

Thanks for your review! I pushed some changes to address the issue with keyboard navigation and selection.

@dhayab dhayab force-pushed the angular-12-query-suggestions branch from 69b30a7 to faaeab2 Compare October 25, 2021 16:07
francoischalifour
francoischalifour approved these changes Oct 25, 2021
View reviewed changes
Angular InstantSearch/query-suggestions/src/app/autocomplete.component.ts
return querySuggestion.query;
}

handleKeyUp(event: KeyboardEvent) {
Copy link
Member

@francoischalifour francoischalifour Oct 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is the Angular (keyup) equivalent to the input event? (onChange in React)

It feels weird to use keyup to track value changes. It would make more sense to use keyup to stop the arrow propagation and change to refine.

Not blocking though since it was like this before this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

In Angular (keyup) and (change) are equivalent to their counterpart in JS (keyup - change), meaning that I would have to commit the value by removing focus from the input to trigger the change using the latter event.

sarahdayan
sarahdayan reviewed Oct 25, 2021
View reviewed changes
Angular InstantSearch/query-suggestions/package.json Outdated
Comment on lines 13 to 42
"@angular/animations": "~12.2.0",
"@angular/cdk": "12.2.11",
"@angular/common": "~12.2.0",
"@angular/compiler": "~12.2.0",
"@angular/core": "~12.2.0",
"@angular/forms": "~12.2.0",
"@angular/material": "12.2.11",
"@angular/platform-browser": "~12.2.0",
"@angular/platform-browser-dynamic": "~12.2.0",
"@angular/router": "~12.2.0",
"algoliasearch": "^4.10.5",
"angular-instantsearch": "4.0.1",
"rxjs": "~6.6.0",
"tslib": "^2.3.0",
"zone.js": "~0.11.4"
},
"devDependencies": {
"@angular-devkit/build-angular": "0.13.8",
"@angular/cli": "7.3.8",
"@angular/compiler-cli": "6.1.10",
"@angular/language-service": "6.1.10",
"@types/algoliasearch": "3.30.13",
"@types/jasmine": "3.3.12",
"@types/jasminewd2": "2.0.6",
"@types/node": "8.10.47",
"codelyzer": "5.1.0",
"jasmine-core": "2.99.1",
"jasmine-spec-reporter": "4.2.1",
"karma": "4.1.0",
"karma-chrome-launcher": "2.2.0",
"karma-coverage-istanbul-reporter": "2.0.5",
"karma-jasmine": "2.0.1",
"karma-jasmine-html-reporter": "0.2.2",
"prettier": "1.17.1",
"protractor": "6.0.0",
"ts-node": "8.1.0",
"tslint": "5.17.0",
"typescript": "2.8.4"
"@angular-devkit/build-angular": "~12.2.3",
"@angular/cli": "~12.2.3",
"@angular/compiler-cli": "~12.2.0",
"@types/algoliasearch": "^4.0.0",
"@types/jasmine": "~3.8.0",
"@types/node": "^12.11.1",
"jasmine-core": "~3.8.0",
"karma": "~6.3.0",
"karma-chrome-launcher": "~3.1.0",
"karma-coverage": "~2.0.3",
"karma-jasmine": "~4.0.0",
"karma-jasmine-html-reporter": "~1.7.0",
"typescript": "~4.3.5"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we no longer pin dependencies?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure, the package.json from the Angular InstantSearch template in create-instantsearch-app is in this state. Should I fix the versions in both the CLI and this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, Angular seems to be the only non-pinned template. Maybe you can pin this example and handle the template pinning and the pinning of the other examples in separate PRs.

Angular InstantSearch/query-suggestions/src/app/app.component.css Outdated Show resolved Hide resolved
Angular InstantSearch/query-suggestions/src/app/app.component.html Show resolved Hide resolved
Angular InstantSearch/query-suggestions/src/app/autocomplete.component.ts Outdated Show resolved Hide resolved
@dhayab dhayab force-pushed the angular-12-query-suggestions branch 2 times, most recently from e320898 to 6c50ee6 Compare October 26, 2021 10:05
sarahdayan
sarahdayan reviewed Oct 26, 2021
View reviewed changes
Copy link
Member

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

FYI I couldn't preview the demo in CodeSandbox because of some error. Same in private browsing, and on every browser I have installed. Might be worth checking before merging.

Capture d’écran 2021-10-26 à 12 14 29

@dhayab
Copy link
Member Author

dhayab commented Oct 26, 2021

@sarahdayan I looked at it and it looks like Codesandbox has a problem with the satellite.css styles we're adding in our app using the angular.json config file.

I have been able to reproduce the issue in a minimal sandbox (using the official Angular template) here: https://codesandbox.io/s/elated-banach-j6hiv?file=/.angular-cli.json. It also exists in the previous version of this sample: https://codesandbox.io/s/github/algolia/doc-code-samples/tree/master/Angular%20InstantSearch/query-suggestions.

We could probably move forward with the merge, and I'll create an issue on CSB github repo to let them know of this problem.

What do you think?

@dhayab dhayab force-pushed the angular-12-query-suggestions branch from 6c50ee6 to 1270673 Compare October 26, 2021 15:04
@sarahdayan
Copy link
Member

We could probably move forward with the merge, and I'll create an issue on CSB github repo to let them know of this problem.

Absolutely, just wanted to make sure it's on the radar 👍

@dhayab dhayab merged commit ba68a9f into master Oct 26, 2021
@dhayab dhayab deleted the angular-12-query-suggestions branch October 26, 2021 15:19
@dhayab dhayab mentioned this pull request Oct 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francoischalifour francoischalifour francoischalifour approved these changes

@sarahdayan sarahdayan sarahdayan approved these changes

@tkrugg tkrugg Awaiting requested review from tkrugg tkrugg was automatically assigned from algolia/frontend-experiences-web

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhayab @francoischalifour @sarahdayan