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(search): populate Search field with user provided value #1019

Merged
merged 1 commit into from Jul 13, 2017
Merged

fix(search): populate Search field with user provided value #1019

merged 1 commit into from Jul 13, 2017

Conversation

thom4parisot
Copy link
Contributor

@thom4parisot thom4parisot commented Jul 10, 2017

This pull request populates the query field when the Vue component is initialised.

The Good

It works.

The Bad

I have not found any particular documentation for $options.el so it might break when upgrading Vue.js.

There are probably better ways to do:

  • passing a value="..." property to the search field as a prop (could not figure out how to pass the paramter to the search macro)
  • maybe a more official way to bind to the original input#search field

The Ugly

The Vue component relies on the DOM of the original so it can break just by changing the HTML template.

What's next?

Once this issue is fixed, I would like to remove the code duplication between the HTML view and the Vue component. Either by extending the form and providing only the autocomplete feature.

fix #1016

Copy link
Contributor

@abulte abulte left a comment

Choose a reason for hiding this comment

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

We need to try something more "official" like you say before merging, I'll have a look at it tomorrow!

@thom4parisot
Copy link
Contributor Author

@abulte yeah, exactly. I kind of wanted to start the conversation with the most obvious and minimal change prior to jump in rewriting the component ;-)

I'll be away in user interviews with @thimy tomorrow — but should be online between 2pm and 3:30pm 🙂

@abulte
Copy link
Contributor

abulte commented Jul 10, 2017

@oncletom 👍

@abulte
Copy link
Contributor

abulte commented Jul 11, 2017

@oncletom do you have a reliable way of testing this? Because I'm having trouble :-/ You may try the code below. I'm hoping the null won't erase the existing DOM value (as opposed to empty string), but I'm not sure.

    created() {
      if (this.$location.query.q) {
        this.query = decodeURIComponent(this.$location.query.q.replace('+', ' '));
      }
    },
    data() {
        // const query = this.$location.query.q || '';
        return {
            current: -1,
            query: null,
            show: false,
            cache: new Cache('site-search', sessionStorage),
            groups: [
                group('datasets', this._('Datasets')),
                group('reuses', this._('Reuses')),
                group('organizations', this._('Organizations'), 'organization'),
                group('territory', this._('Territories'), 'territory'),
            ],
            minLength: 2,
            placeholders: placeholders,
        }
    },

@thom4parisot
Copy link
Contributor Author

@abulte I will give a go at it tomorrow (will be in the office) 👋 if you are around — I agree it would be nicer to compute the field value only once (#created()) instead of at anytime (#data()), and prior to the user first interaction with the component.

const originalField = this.$options.el.querySelector('#search');
const queryStringValue = decodeURIComponent(query.q.replace('+', ' '));

this.query = originalField.value || queryStringValue || '';
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 does the trick. We either pick:

  1. the value at runtime of the search field (altered by user or provided by a input[value] attribute;
  2. otherwise we pick it up from the query string;
  3. otherwise field value has not been setup

@thom4parisot
Copy link
Contributor Author

@abulte I have made the change 😃

Copy link
Contributor

@abulte abulte left a comment

Choose a reason for hiding this comment

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

Missing CHANGELOG but OK.

@thom4parisot
Copy link
Contributor Author

Changelog updated 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants