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

Update ui.js #216

Merged
merged 6 commits into from
Apr 11, 2022
Merged

Update ui.js #216

merged 6 commits into from
Apr 11, 2022

Conversation

satelines
Copy link
Contributor

Enable to add ID for search automcplete in order to enable more than one search element on same page

Enable to add ID for search automcplete in order to enable more than one search element on same page
config = {field: defaults.field, current: config};
}

Object.assign(defaults, config);
Copy link
Owner

Choose a reason for hiding this comment

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

that looks wrong. According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign this copies from config to defaults

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that at the begining, and it didn't work. this works as expected.

Copy link
Owner

Choose a reason for hiding this comment

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

var defaults = {a: 1, b: 2},
    config = {b: 4};

Object.assign(defaults, config);

console.log(config);

Output:

{b: 4}

No default has been merged into config


var field = $('#org_openpsa_search_query'),
var field = $(config.field),
Copy link
Owner

Choose a reason for hiding this comment

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

what about line 157/158? There's also hardcoded selectors down there

assign config to result of object.assign, in order to preserve all keys
Change id to class, in order to avoid duplicate id in case multiple search elements will be display on same page and adjust selectors
li_class += ' current';
enable_provider(provider);
}

$('<li class="' + li_class + '">' + provider.placeholder + '</li>')
.data('provider', provider)
.click(function() {
var old_item = $('#org_openpsa_search_providers .current'),
query = $('#org_openpsa_search_query');
var old_item = form.find('.org_openpsa_search_providers .current');
Copy link
Owner

Choose a reason for hiding this comment

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

if this changes from id to classname, this also has to change: https://github.com/flack/openpsa/search?q=org_openpsa_search_trigger

Change id to class selector
Solve css issur caused from changing selctors from id to class
@@ -432,7 +432,8 @@ ul.org_openpsa_search_providers
box-shadow: .1em .2em .5em rgba(0, 0, 0, .3);
}

ul.org_openpsa_search_providers li
form[id^="org_openpsa_search_"] ul.org_openpsa_search_providers li,
#org_openpsa_search_form ul.org_openpsa_search_providers li
Copy link
Owner

Choose a reason for hiding this comment

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

How about #content-menu ul.org_openpsa_search_providers li instead?

Copy link
Owner

Choose a reason for hiding this comment

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

I mean, instead of two separate selectors. the effect should be the same, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the first doesn't work with container contet-menu, it is for pages that hast different form id, and don't containt menu-content id

@flack flack merged commit cb64b62 into flack:master Apr 11, 2022
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