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

COM- 3305 Added string method to check special char #310

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

SharmaBhushan
Copy link
Contributor

No description provided.

@@ -206,6 +206,9 @@ define([
filter: function(collection) {
var filterStr = "";
var qName = $("#searchName").val();
if (typeof String.prototype.mzCheckSpacialChar === 'function') {
Copy link
Contributor

@sagarraut0007 sagarraut0007 Mar 17, 2021

Choose a reason for hiding this comment

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

since mzCheckSpacialChar is part of SDK then it will always be available. so no need to check this is a function (mzCheckSpacialChar ) is available or not but not 100% sure about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added this for safer side if function is not loaded by sdk current search functionality should work, but in other side if SDK will not load mozu application will not work so I am fine to remove this if condition.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I don't think this is the best way to fix such a common use case. If we are adding a generic filter escape method we should hook it into a generic place and fix all filter request strings. Not just this one place.

See my comments on the Storefront PR. That has more details.

Base automatically changed from feature/COM-2911 to feature/B2B-phase-2 April 6, 2021 00:21
@ghost ghost force-pushed the feature/B2B-phase-2 branch from b6dd923 to d880655 Compare April 8, 2021 21:23
Base automatically changed from feature/B2B-phase-2 to develop April 8, 2021 23:15
@ghost ghost closed this May 19, 2021
@ghost
Copy link

ghost commented May 19, 2021

This PR needs to be rebased and integrate the changes from PR #346. I still think the fix there needs some more work, though. Please review my comments again.

@ghost ghost reopened this May 19, 2021
@kibo-DeepakTajane kibo-DeepakTajane force-pushed the feature/COM-3305 branch 2 times, most recently from cf34680 to 6069f65 Compare July 14, 2021 05:48
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.

2 participants