Tami/boolean operators by tamirissg · Pull Request #10 · jusbrasil/JavaScript-autoComplete

tamirissg · 2020-09-09T19:12:31Z

No description provided.

felipemsantana · 2020-09-09T19:57:01Z

auto-complete.css

+.chip
+ {
+    border: 1px solid transparent;
+}
+.chip:hover {
+    border: 1px solid black;
+}


You can't override mystique classes here, this will apply to all chips in the page, replacing the style everywhere.

Create a class for this purpose, like autocomplete-chip.

felipemsantana · 2020-09-09T20:00:23Z

auto-complete.js

+    window.changeText = () => {
+
+        const newText = 
+             `


Do not use ES2015+ syntax features, like arrow function, const or template string. Doing this will break the whole JS script for older browsers, all code from the Topbar JS will not work.

Use ES5 syntax.

felipemsantana · 2020-09-09T20:06:38Z

auto-complete.js

        }

+
+    window.changeText = () => {


Avoid using global object window for this, it's considered bad practice because of the unscoped access, you could easily replace this function in another place and will break this code.

Instead, create a local function

function changeText() { ... }

And add it using JS

var buttons = document.getElementsByClassName('autocomplete-boolean-button'); for (var i = 0; i < buttons.length; i++) { var button = buttons[i]; button.addEventListener('click', changeText); }

felipemsantana · 2020-09-09T20:07:29Z

auto-complete.js

+
+        const newText = 
+             `
+             <div style="margin: 0.4em" class="row">


margin: 0.4em? This does not seem to be correct, check with Predo.

felipemsantana · 2020-09-09T20:08:08Z

auto-complete.js

+        const newText = 
+             `
+             <div style="margin: 0.4em" class="row">
+                <span style="font-size: 10px" type="button" class="chip chip--outline chip--sm" data-toggle="tooltip" data-placement="bottom-right"


type="button" is not a valid attribute here, also, it does not behave as a button.

Suggested change

<span style="font-size: 10px" type="button" class="chip chip--outline chip--sm" data-toggle="tooltip" data-placement="bottom-right"

<span style="font-size: 10px" class="chip chip--outline chip--sm" data-toggle="tooltip" data-placement="bottom-right"

Change in every other span.

felipemsantana · 2020-09-09T20:15:25Z

auto-complete.js

+    }
+        var footerBooleanOperators = `
+            <hr style="margin: 0.4em">
+            <div id= "search" style="margin-left: 0.6em" >


Do not use id, this script will be executed more than once in SERP home for example and the id will conflict.

Suggested change

<div id= "search" style="margin-left: 0.6em" >

<div style="margin-left: 0.6em">

Also, all sizes using em unit seems to be wrong, ask Predo about this.

felipemsantana · 2020-09-09T20:16:23Z

auto-complete.js

+                </span>
+            </div> `  
+
+        document.getElementById('search').innerHTML = newText;           


Avoid using id, this won't work if there is 2 or more elements in the same page with same id. Also, search is too common name to be used for id and likely to conflict.

Suggested change

document.getElementById('search').innerHTML = newText;

this.parentElement.innerHTML = newText;

felipemsantana · 2020-09-09T20:17:44Z

auto-complete.js

+        var footerBooleanOperators = `
+            <hr style="margin: 0.4em">
+            <div id= "search" style="margin-left: 0.6em" >
+                <button class="btn btn--flat btn--blue" style="font-size: 10px" onclick= "changeText()" >


Like I said above

Suggested change

<button class="btn btn--flat btn--blue" style="font-size: 10px" onclick= "changeText()" >

<button class="btn btn--flat btn--blue autocomplete-boolean-button" style="font-size: 10px">

felipemsantana · 2020-09-09T20:18:20Z

auto-complete.js

+                     DICAS PARA ESPECIFICAR SUA BUSCA
+                </button>
+            </div>`


We use 2 spaces for indentation level

Suggested change

DICAS PARA ESPECIFICAR SUA BUSCA

</button>

</div>`

DICAS PARA ESPECIFICAR SUA BUSCA

</button>

</div>`

felipemsantana · 2020-09-09T20:18:36Z

auto-complete.js

+        document.getElementById('search').innerHTML = newText;           
+
+    }
+        var footerBooleanOperators = `


Do not use template string, for the same reason I said above.

felipemsantana

Delete package-lock.json file

felipemsantana · 2020-09-14T18:25:19Z

auto-complete.css

+.row{
+    margin-inline-start: 5px;
+}


Remove this, it's a mystique class

felipemsantana · 2020-09-14T18:27:46Z

auto-complete.css

+    font-size: 10px;
+    line-height: 16px;
+    margin-inline-start: 10px;
+    align-items: center;


This does not work without flex, remove

felipemsantana · 2020-09-14T18:32:51Z

auto-complete.css

+    border-color:black;
+}
+
+.autocomplete-chip::before {


This element does not have any text, also it should keep the mystique default style, remove

felipemsantana · 2020-09-14T18:32:59Z

auto-complete.css

+}
+
+.autocomplete-chip:hover {
+    border-color:black;


Suggested change

border-color:black;

border-color: black;

felipemsantana · 2020-09-14T19:29:20Z

auto-complete.js

+            var top = pos.top + 1;
+            var clonedNode = this.cloneNode(true);
+            clonedNode.style =
+            'color:transparent;box-shadow:none;z-index:10000;position:absolute;left:' + left + 'px;top:' + top + 'px';


Suggested change

'color:transparent;box-shadow:none;z-index:10000;position:absolute;left:' + left + 'px;top:' + top + 'px';

'color:transparent;box-shadow:none;z-index:10000;position:absolute;left:' + left + 'px;top:' + top + 'px';

felipemsantana · 2020-09-14T19:29:42Z

auto-complete.js

+            'color:transparent;box-shadow:none;z-index:10000;position:absolute;left:' + left + 'px;top:' + top + 'px';
+
+            addEvent(clonedNode, 'mouseout', function () {
+


Where is the remove child function?

felipemsantana · 2020-09-14T19:30:45Z

tests/auto-complete.js

+
+})
+
+     function renderText(tooltip, text) {


What is this?

felipemsantana and others added 3 commits August 17, 2020 13:57

Init boolean operators feature

0ad5a22

boolean search

9939561

boolean search

02ecf21

felipemsantana suggested changes Sep 9, 2020

View reviewed changes

tamirissg and others added 9 commits September 10, 2020 14:16

refactoring and correcting

7dd8fd2

Fix tooltips

a85102a

Add enableFooterBooleanOperators option

6a82ee5

refactoring and correcting

7bf6241

refactoring and correcting

dfdc314

tests

bffc2d8

tests

0788af3

boolean search tests

6b90e92

tooltip-lg implementation

fff0120

felipemsantana reviewed Sep 14, 2020

View reviewed changes

	<span style="font-size: 10px" type="button" class="chip chip--outline chip--sm" data-toggle="tooltip" data-placement="bottom-right"
	<span style="font-size: 10px" class="chip chip--outline chip--sm" data-toggle="tooltip" data-placement="bottom-right"

	<div id= "search" style="margin-left: 0.6em" >
	<div style="margin-left: 0.6em">

	document.getElementById('search').innerHTML = newText;
	this.parentElement.innerHTML = newText;

	<button class="btn btn--flat btn--blue" style="font-size: 10px" onclick= "changeText()" >
	<button class="btn btn--flat btn--blue autocomplete-boolean-button" style="font-size: 10px">

	'color:transparent;box-shadow:none;z-index:10000;position:absolute;left:' + left + 'px;top:' + top + 'px';
	'color:transparent;box-shadow:none;z-index:10000;position:absolute;left:' + left + 'px;top:' + top + 'px';

		'color:transparent;box-shadow:none;z-index:10000;position:absolute;left:' + left + 'px;top:' + top + 'px';

		addEvent(clonedNode, 'mouseout', function () {

Conversation

tamirissg commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipemsantana left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants