-
Notifications
You must be signed in to change notification settings - Fork 35
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(quantic): QuanticResultsPerPage fixes and refactor tests #4889
Conversation
Pull Request ReportPR Title❌ Title should follow the conventional commit spec: Example: Live demo linksBundle Size
SSR Progress
Detailed logssearch : buildInteractiveResultsearch : buildInteractiveInstantResult search : buildInteractiveRecentResult search : buildInteractiveCitation search : buildGeneratedAnswer recommendation : missing SSR support case-assist : missing SSR support insight : missing SSR support commerce : missing SSR support |
@erocheleau to avoid issues with the versioning we should update the PR title to |
packages/quantic/force-app/main/default/lwc/quanticResultsPerPage/quanticResultsPerPage.js
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticResultsPerPage/quanticResultsPerPage.js
Show resolved
Hide resolved
...tic/force-app/main/default/lwc/quanticResultsPerPage/__tests__/quanticResultsPerPage.test.js
Outdated
Show resolved
Hide resolved
...tic/force-app/main/default/lwc/quanticResultsPerPage/__tests__/quanticResultsPerPage.test.js
Outdated
Show resolved
Hide resolved
...tic/force-app/main/default/lwc/quanticResultsPerPage/__tests__/quanticResultsPerPage.test.js
Outdated
Show resolved
Hide resolved
...tic/force-app/main/default/lwc/quanticResultsPerPage/__tests__/quanticResultsPerPage.test.js
Outdated
Show resolved
Hide resolved
...tic/force-app/main/default/lwc/quanticResultsPerPage/__tests__/quanticResultsPerPage.test.js
Show resolved
Hide resolved
...tic/force-app/main/default/lwc/quanticResultsPerPage/__tests__/quanticResultsPerPage.test.js
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticResultsPerPage/e2e/fixture.ts
Show resolved
Hide resolved
packages/quantic/force-app/main/default/lwc/quanticResultsPerPage/e2e/pageObject.ts
Outdated
Show resolved
Hide resolved
...es/quantic/force-app/main/default/lwc/quanticResultsPerPage/e2e/quanticResultsPerPage.e2e.ts
Show resolved
Hide resolved
...es/quantic/force-app/main/default/lwc/quanticResultsPerPage/e2e/quanticResultsPerPage.e2e.ts
Show resolved
Hide resolved
Very true, I forgot! thanks! |
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.
After addressing mehdi's concerns and comments, it seems good to me!
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.
Looking good, GG!
SFINT-5851
IN THIS PR:
quanticResultsPerPage
component from Cypress to Playwright and adding unit tests.initialChoice
or noinitialChoice
is passed to the component. We saidBut in reality, we were always defaulting to the value
10
whenever no value was passed,And if the value was not within the
choicesDisplayed
we would completely crash Quantic initialization by throwing an error.Instead now, if the value is invalid (such as not a number, missing or not part of the
choicesDisplayed
) we actually default to the first value inchoicesDisplayed
. Log an error in the console, but do not throw.If the values in
choicesDisplayed
is invalid, we log an error in the console, and display the componentError, but no longer throw an error.UNIT TESTS:
E2E TESTS: