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 gh-1934: Search bar special chars #2075

Merged

Conversation

@chen-robert
Copy link
Contributor

commented Sep 8, 2018

Resolves:

Resolves #1934

The text was encoded once already by the search bar. That means we need to decode it twice in order to decode special characters.

Test Coverage:

?q=cat%3Fcat (should now appear as cat?cat)
?q=cat%20cat (should still appear as cat cat)

Fixed Issue 1934
The text was encoded once already by the search bar. That means we need to decode it twice in order to decode special characters.
@benjiwheeler

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

This is awesome!

Using your code change, when I put ?q=cat%20cat into the search box and press enter, it does indeed convert the %20 to a space. However, it does not actually do anything about the ?q= part -- so the result is "?q=cat cat". I still need to click on the Studios button/tab to get it to be "cat cat".

(I'm trying the replication instructions in #1934, FYI.)

search prob

Here's what I think will fix it. We need to repeat the entire set of steps that strip the search syntax away, not just the decodeURIComponent function. So we can make a function like

    decodeSearchQuery (query, key) {
        const keyIndex = query.lastIndexOf(`${key}=`);
        let term = query; // note that this was initing to '' before, but then if the 'q=' key isn't found, it returns a blank string. I think it's better to maybe return the query string as is if the key isn't found?
        if (keyIndex !== -1) {
            term = query.substring(keyIndex + key.length + 1, query.length).toLowerCase();
        }
        while (term.indexOf('/') > -1) {
            term = term.substring(0, term.indexOf('/'));
        }
        while (term.indexOf('&') > -1) {
            term = term.substring(0, term.indexOf('&'));
        }
        term = decodeURIComponent(term.split('+').join(' '));
        return term;
    }

that we can call from componentDidMount twice, like

    componentDidMount () {
        const query = window.location.search;
        const term = this.decodeSearchQuery(this.decodeSearchQuery(query, 'q'), 'q');
        this.props.dispatch(navigationActions.setSearchTerm(term));
    }

This should make the string resolve completely, the first time:

search fix

The 'key' parameter allows us to use this for handling the mode query key-value in the constructor as well, like:

        let mode = '';
        const query = window.location.search;
        mode = this.decodeSearchQuery(query, 'mode');
        if (ACCEPTABLE_MODES.indexOf(mode) !== -1) {
            this.state.mode = mode;
        }

Want to try that, or something like it, and test further?

@benjiwheeler benjiwheeler removed their assignment Sep 13, 2018

@benjiwheeler benjiwheeler self-requested a review Sep 13, 2018

@chen-robert

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

Oh, I think I misinterpreted the bug report.

To clarify,
?q=cat%3Fcat appears as cat?cat
What about cat%3Fcat alone? Should it be cat?cat or cat%3Fcat.
Also, what other flags are there? Should I translate ?q=cat%3Fcat as well as aliases such as ?query=cat%3Fcat?

@chen-robert

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2018

@benjiwheeler I've implemented the changes. I'm not sure if typing ?q=${searchText} is user friendly though. Is that feature intended?

@chen-robert

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@rschamp I've fixed the pull request. How should I proceed? It looks like its been sitting here for quite some time.

@rschamp

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

@chen-robert Sorry about that! @benjiwheeler will re-review soon.

@benjiwheeler

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

Sorry for the delay, @chen-robert! This looks good to me, and I tested it and it works great.

I think I misinterpreted the bug report too -- and the gif there was a bit confusing. You're right, we shouldn't really be worrying about users actually manually entering "?q=cat%20cat" into the search box!

Thanks for this!

@benjiwheeler benjiwheeler merged commit a94b9d6 into LLK:develop Sep 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.