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

School dropdown: fix selection problem #24481

Merged
merged 9 commits into from Aug 28, 2018
Merged

Conversation

breville
Copy link
Member

@breville breville commented Aug 26, 2018

The school dropdown was exhibiting an interesting problem in its use on various pages, including https://studio.code.org/users/sign_up, https://hourofcode.com/, and the teacher mini-census on https://studio.code.org/home: once an option was selected in the dropdown, it wouldn't appear in the box; it would only appear after clicking outside of the box.

It turns out that SchoolAutocompleteDropdown (and SchoolAutocompleteDropdownWithLabel) support two properties specifying the selected school. One, successfully used by https://code.org/yourschool, provides a schoolDropdownOption object containing both a value and a label, the latter to be displayed. The other, used by the problematic pages, only provides the value. Unfortunately, upon re-rendering with a refreshed property, the 1.x react-select control doesn't attempt to retrieve the relevant label asynchronously, and so nothing would display.

(Why did it show something after clicking elsewhere on the page? react-select appears to request its options at this point, and we oblige with a new Ajax request that retrieves the school information for the single entry that it wants to know about, and that's enough for us to generate the label.)

Early attempts at a solution (seen in early commits in this PR) required users of the dropdown to always provide the label as well as the value, but it seemed nicer to not require changes to any existing users.

The new solution is for the dropdown to cache the most recently-selected value and label, and that way the label is ready for display upon the successful re-render that immediately follows.

broken:
school_dropdown_broken

working:
school_dropdown_working

@drewsamnick
Copy link
Contributor

This seems to fix the problem as described, but I wonder if there is actually something else going on. Why does the label even need to be reloaded after the option is selected? It should have already been loaded in order for you to select it.

@breville
Copy link
Member Author

breville commented Aug 28, 2018

@drewsamnick I'm glad you asked, and I spent an exciting day debugging the control. (It was extra fun using the distribution JS source code, while the Chrome debugger had all source file mapping off by one line.)

Long story short, it's a known issue with the control, and I had fortunately already arrived at the suggested workaround: JedWatson/react-select#865

As the write-up describes, selecting an entry clears the user's input, which causes the control to request the set of options for an empty string, which for us is nothing since we have no default set of options. As we attempt to render a couple times immediately after that selection, we first hit a "loading" state which prevents the selected option from showing, and then we hit a state in which we've flushed the set of options (i.e. the list of schools that had just been showing in the dropdown), and so the passed-in value can't do anything by itself.

(As for why it's able to load the label when we provide an initial value, the control has a separate codepath to do that during initial mount.)

render() {
// value will end up either an object or a string, depending whether we have
// a label or not. It appears to be the quirky behavior of react-select 1.x.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we include the link to JedWatson/react-select#865 here, so we can potentially notice and remove this workaround when the issue is resolved in the underlying library?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea; done. That said, react-select was completely rewritten for v2.x, and upgrading to that is a bigger impact work item for us, so I don't think we should expect a fix anytime soon.

@breville breville changed the title School dropdown: fix selection problem on sign-up School dropdown: fix selection problem Aug 28, 2018
@breville breville merged commit b1ebc72 into staging Aug 28, 2018
@breville breville deleted the school-dropdown-signup-fix branch August 28, 2018 21:26
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.

None yet

5 participants