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

Stop triggering autocomplete for non-auto complete fields. Support data attribute based autocomplete types. #4449

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

orangewolf
Copy link
Member

Moves to a new autocomplete type choosing system that uses data[autocomplete-type] to declaritively select
the type instead of just keying of certain field names. Does not remove the existing functionality, but is
a baby step toward depricating it.

Fixes #3510 ; refs #99 (maybe?)

Guidance for testing, such as acceptance criteria or new user interface behaviors:

  • There are no specs around the autocomplete js as is, so none were modified or edited
  • Only real way to test the autocomplete getting called all the time is with a JS breakpoint or debugger statement.
    Here's a video that shows it happening on Hyku running on vanilla Hyrax 2.5.1 https://share.getcloudapp.com/d5uWdzZj (sorry about the fan noise my mac is prepping for launch)
  • Adding a new field and making it linked data can be done by adding a helper like so to the app/views/records/edit_fields/_language.html.erb file for example
<%= f.input key,
      as: :multi_value,
      input_html: {
        class: 'form-control',
        data: {
          'autocomplete-type' => 'linked',
          'autocomplete-url' => "/authorities/search/local/languages",
          'autocomplete' => key
        }
      },
 required: f.object.required?(key) %>

@samvera/hyrax-code-reviewers

} else if(type === 'linked') {
new LinkedData(element, url)
} else {
new Default(element, url)

Choose a reason for hiding this comment

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

Do not use 'new' for side effects no-new

element,
url)
} else if(type === 'linked') {
new LinkedData(element, url)

Choose a reason for hiding this comment

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

Do not use 'new' for side effects no-new

{ excluding: exclude }
)
} else if(type == 'resource' ) {
new Resource(

Choose a reason for hiding this comment

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

Do not use 'new' for side effects no-new

url,
{ excluding: exclude }
)
} else if(type == 'resource' ) {

Choose a reason for hiding this comment

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

Expected '===' and instead saw '==' eqeqeq

let type = element.data('autocomplete-type')
let exlude = element.data('exclude-work')
if(type === 'resource' && exclude.length > 0) {
new Resource(

Choose a reason for hiding this comment

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

Do not use 'new' for side effects no-new

…ta attribute based autocomplete types.

Moves to a new autocomplete type choosing system that uses data[autocomplete-type] to declaritively select
the type instead of just keying of certain field names. Does not remove the existing functionality, but is
a baby step toward depricating it.
@jeremyf
Copy link
Contributor

jeremyf commented Jul 16, 2020

Is this something that should be added to the master branch as well?

@jeremyf jeremyf merged commit 0cccaf3 into samvera:2.x-stable Jul 16, 2020
@no-reply
Copy link
Contributor

@jeremyf:

Is this something that should be added to the master branch as well?

for future reference, our branching policy is to always merge fixes to the mainline before pushing them to maintenance branches. the only exception should be cases where the fix is no longer relevant on the mainline, but resolves a significant issue for the maintenance series. in other words: we should always be backporting, never forward-porting. this is to avoid complicated regression situations, where fixes released on maintenance branches aren't available on current releases.

we should either:

  • immediately forward port this, reconciling the current state; or, if that's not possible
  • revert this merge, rework for master, and backport from there using the usual process.

@orangewolf
Copy link
Member Author

@no-reply that's my bad, as I didn't know that we were only backporting. That's a bit of a lift, as I don't have anything local running against master at the moment, but I can take a swing at getting this done for master later today if no one else can tackle it.

@no-reply
Copy link
Contributor

no worries! i'm delighted (and relieved) to have this fix in. we'll get the branch reconciled in short order.

this might not have been the platonic ideal of the process, but it looks like the system working to me!

eltiffster added a commit to UVicLibrary/Vault that referenced this pull request Jun 22, 2022
eltiffster added a commit to UVicLibrary/Vault that referenced this pull request Jun 27, 2022
…s from samvera/hyrax#4449.

Restrict autocomplete changes to controlled vocabulary input fields.
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.

4 participants