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

Changed field of study select to match anywhere in string w/ highlighted text #632

Merged
merged 9 commits into from
Jun 30, 2016

Conversation

gsidebo
Copy link
Contributor

@gsidebo gsidebo commented Jun 24, 2016

What are the relevant tickets?

Fixes #578

What's this PR do?

  1. Changes the field of study autocomplete to match any part of the field of study value (not just prefix)
  2. Highlights the text in the fields of study label that matches the search text
  3. Refactors some SelectField/AutoComplete code to allow for better code reuse and easy declaration of autocomplete behaviors

Where should the reviewer start?

Probably SelectField.js, followed by AutoComplete.js and AutoCompleteSettings.js

How should this be manually tested?

Run through the /profile/education tab. Be sure to fill out fields of study and country/state.

Screenshots (if appropriate)

ss 2016-06-24 at 11 44 56

@bdero bdero temporarily deployed to micromasters-ci-pr-632 June 24, 2016 15:48 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-632 June 24, 2016 15:52 Inactive
@@ -499,60 +521,6 @@ class AutoComplete extends Component {
}
}

AutoComplete.levenshteinDistance = (searchText, key) => {
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 removed lines here were moved to AutoCompleteSettings.js

@bdero bdero temporarily deployed to micromasters-ci-pr-632 June 24, 2016 15:54 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-632 June 24, 2016 16:10 Inactive
@gsidebo gsidebo force-pushed the 578_FoS_match_anywhere_in_string branch from a8653b7 to 4c4221e Compare June 24, 2016 16:11
@bdero bdero temporarily deployed to micromasters-ci-pr-632 June 24, 2016 16:11 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-632 June 24, 2016 17:25 Inactive
@noisecapella noisecapella self-assigned this Jun 27, 2016
AutocompleteSettings.noFilter = () => true;

AutocompleteSettings.defaultFilter = AutocompleteSettings.caseInsensitivePrefixFilter = (searchText, key) => {
let index = key.toLowerCase().indexOf(searchText.toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically outside the scope of this PR but can you replace this with key.toLowerCase().startsWith(searchText.toLowerCase())?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

);
return renderTestComponent(SelectField, selectProps);
let renderLetterSelectField = () => {
_.extend(inputProps, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Object.assign here?

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'm just trying to add properties to an existing object. to accomplish that, i'd need to do something awkward like inputProps = Object.assign({}, inputProps, {}). _.extend is meant for this exact purpose

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Object.assign(inputProps, {...}) then? At least for our purposes it's the same function, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, i misunderstood Object.assign. fixed

@bdero bdero had a problem deploying to micromasters-ci-pr-632 June 27, 2016 16:17 Failure
@bdero bdero temporarily deployed to micromasters-ci-pr-632 June 27, 2016 16:35 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-632 June 27, 2016 18:33 Inactive
};
};

export default AutoCompleteSettings;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should export each of the functions above instead of exporting this object here. We will never use this object except to retrieve its functions

@noisecapella
Copy link
Contributor

Can you rebase please?

@noisecapella
Copy link
Contributor

Code looks good, I'll just check the functionality once more after rebase

@noisecapella
Copy link
Contributor

Looks great 👍

@gsidebo gsidebo merged commit a350943 into master Jun 30, 2016
@gsidebo gsidebo deleted the 578_FoS_match_anywhere_in_string branch June 30, 2016 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants