-
Notifications
You must be signed in to change notification settings - Fork 4
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
[CSL-3108] Trim space adjustments #183
Conversation
src/modules/autocomplete.js
Outdated
const queryTrimmed = helpers.trimNonBreakingSpaces(query); | ||
|
||
// Validate query (term) is provided and consists of more than non-breaking spaces | ||
if (!queryTrimmed || typeof queryTrimmed !== 'string') { |
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.
This was added to ensure we don't dispatch requests consisting of solely non-breaking spaces (ex:
)
src/modules/search.js
Outdated
const queryTrimmed = helpers.trimNonBreakingSpaces(query); | ||
|
||
// Validate query (term) is provided and consists of more than non-breaking spaces | ||
if (!queryTrimmed || typeof queryTrimmed !== 'string') { |
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.
This was added to ensure we don't dispatch requests consisting of solely non-breaking spaces (ex:
)
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.
This looks great to me! 🚀 Just left one minor comment that you can feel free to ignore.
Thanks for working on this! ❤️
spec/src/modules/search.js
Outdated
@@ -6,6 +6,7 @@ const sinon = require('sinon'); | |||
const sinonChai = require('sinon-chai'); | |||
const ConstructorIO = require('../../../test/constructorio'); // eslint-disable-line import/extensions | |||
const helpers = require('../../mocha.helpers'); | |||
const utilsHelpers = require('../../../src/utils/helpers'); |
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.
Feel free to ignore this, but should we just import the specific function here so we don't have helpers and utilsHelpers? Haha
i.e. const { trimNonBreakingSpaces } = require('../../../src/utils/helpers');
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.
Great callout - this is going to go away anyway. We should not be trimming on search either
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.
Nice, this is looking great to me!
I noticed that we cleaned the query on autocomplete, but not for search. Don't recall why we did that, but maybe something to revisit in the future.
It has been raised that autocomplete requests should not have non-breaking spaces trimmed from queries (terms). This pull request aims to do the following:
a. No longer trim non-breaking spaces from autocomplete and search requests
b. Add tests to cover the above case.
Reference ticket: https://linear.app/constructor/issue/CSL-3108/investigate-error-with-space-input-in-constructorio-node