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

fix(searchbox): fix usage of custom reset template #2585

Merged
merged 7 commits into from
Nov 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions dev/app/builtin/stories/search-box.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,27 @@ export default () => {
);
})
)
.add(
'with custom templates',
wrapWithHits(container => {
window.search.addWidget(
instantsearch.widgets.searchBox({
container,
placeholder: 'Search for products',
poweredBy: true,
magnifier: {
template: '<div class="ais-search-box--magnifier">🔍</div>',
},
reset: {
template: '<div class="ais-search-box--reset">✖️</div>',
},
templates: {
poweredBy: 'Algolia',
},
})
);
})
)
.add(
'search on enter',
wrapWithHits(container => {
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"scripts": {
"build": "./scripts/build.sh",
"dev": "./scripts/dev.sh",
"dev-novel": "NODE_ENV=development webpack-dev-server --config dev/webpack.config.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably reuse this command from the dev.sh script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? This line was extracted from dev.sh.

Copy link
Contributor

@samouss samouss Nov 20, 2017

Choose a reason for hiding this comment

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

I mean in the dev.sh we can do yarn dev-novel instead of having the same command in two different places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not a big improvement though. Let's iterate on that later.

"doctoc": "doctoc --no-title --maxlevel 3 README.md CONTRIBUTING.md",
"lint": "npm run lint:js && npm run lint:css",
"lint:js": "eslint .",
Expand Down
10 changes: 6 additions & 4 deletions src/widgets/search-box/__tests__/search-box-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ describe('searchBox()', () => {
widget.init(defaultInitOptions);

// Then
expect($('button[type="reset"]')[0].style.display).toBe('none');
expect($('.ais-search-box--reset-wrapper')[0].style.display).toBe('none');
});

it('should be shown when there is a query', () => {
Expand All @@ -233,7 +233,9 @@ describe('searchBox()', () => {
simulateInputEvent('test', 'tes', widget, helper, state, container);

// Then
expect($('button[type="reset"]')[0].style.display).toBe('block');
expect($('.ais-search-box--reset-wrapper')[0].style.display).toBe(
'block'
);
});

it('should clear the query', () => {
Expand All @@ -243,7 +245,7 @@ describe('searchBox()', () => {
simulateInputEvent('test', 'tes', widget, helper, state, container);

// When
$('button[type="reset"]')[0].click();
$('.ais-search-box--reset-wrapper')[0].click();
Copy link
Contributor

@Haroenv Haroenv Nov 20, 2017

Choose a reason for hiding this comment

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

the button is still type reset right? Not sure from this change

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. The problem is that it is in the template, so we cannot rely on what's inside it for the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the source of the bug. If you were to use an emoji, there would not be a button (with type=reset) so instead the event listener is on the outer span.


// Then
expect(helper.setQuery.called).toBe(true);
Expand Down Expand Up @@ -277,7 +279,7 @@ describe('searchBox()', () => {
widget.init(defaultInitOptions);

// Then
expect($('button[type="reset"]').length).toEqual(0);
expect($('.ais-search-box--reset-wrapper').length).toEqual(0);
});
});

Expand Down
50 changes: 38 additions & 12 deletions src/widgets/search-box/search-box.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,12 @@ const renderer = ({
}

if (reset) {
const resetButtonContainer =
containerNode.tagName === 'INPUT'
? containerNode.parentNode
: containerNode;

const resetBtnSelector = `.${cx(bem('reset-wrapper'))}`;
// hide reset button when there is no query
const resetButton = resetButtonContainer.querySelector(
'button[type="reset"]'
);
const resetButton =
containerNode.tagName === 'INPUT'
? containerNode.parentNode.querySelector(resetBtnSelector)
: containerNode.querySelector(resetBtnSelector);
resetButton.style.display = query && query.trim() ? 'block' : 'none';
}
};
Expand Down Expand Up @@ -336,6 +333,16 @@ function addDefaultAttributesToInput(placeholder, input, query, cssClasses) {
CSSClassesToAdd.forEach(cssClass => input.classList.add(cssClass));
}

/**
* Adds a reset element in the searchbox widget. When this reset element is clicked on
* it should reset the query.
* @private
* @param {HTMLElement} input the DOM node of the input of the searchbox
* @param {object} reset the user options (cssClasses and template)
* @param {object} $2 the default templates
* @param {function} clearFunction function called when the element is activated (clicked)
* @returns {undefined} returns nothing
*/
function addReset(input, reset, { reset: resetTemplate }, clearFunction) {
reset = {
cssClasses: {},
Expand All @@ -348,7 +355,7 @@ function addReset(input, reset, { reset: resetTemplate }, clearFunction) {
cssClasses: resetCSSClasses,
});

const htmlNode = createNodeFromString(stringNode);
const htmlNode = createNodeFromString(stringNode, cx(bem('reset-wrapper')));
input.parentNode.appendChild(htmlNode);

htmlNode.addEventListener('click', event => {
Expand All @@ -357,6 +364,14 @@ function addReset(input, reset, { reset: resetTemplate }, clearFunction) {
});
}

/**
* Adds a magnifying glass in the searchbox widget
* @private
* @param {HTMLElement} input the DOM node of the input of the searchbox
* @param {object} magnifier the user options (cssClasses and template)
* @param {object} $2 the default templates
* @returns {undefined} returns nothing
*/
function addMagnifier(input, magnifier, { magnifier: magnifierTemplate }) {
magnifier = {
cssClasses: {},
Expand All @@ -371,10 +386,21 @@ function addMagnifier(input, magnifier, { magnifier: magnifierTemplate }) {
cssClasses: magnifierCSSClasses,
});

const htmlNode = createNodeFromString(stringNode);
const htmlNode = createNodeFromString(
stringNode,
cx(bem('magnifier-wrapper'))
);
input.parentNode.appendChild(htmlNode);
}

/**
* Adds a powered by in the searchbox widget
* @private
* @param {HTMLElement} input the DOM node of the input of the searchbox
* @param {object} poweredBy the user options (cssClasses and template)
* @param {object} templates the default templates
* @returns {undefined} returns nothing
*/
function addPoweredBy(input, poweredBy, templates) {
// Default values
poweredBy = {
Expand Down Expand Up @@ -408,9 +434,9 @@ function addPoweredBy(input, poweredBy, templates) {

// Crossbrowser way to create a DOM node from a string. We wrap in
// a `span` to make sure we have one and only one node.
function createNodeFromString(stringNode) {
function createNodeFromString(stringNode, rootClassname = '') {
const tmpNode = document.createElement('div');
tmpNode.innerHTML = `<span>${stringNode.trim()}</span>`;
tmpNode.innerHTML = `<span class="${rootClassname}">${stringNode.trim()}</span>`;
return tmpNode.firstChild;
}

Expand Down