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 Info Interstitial: country dropdown bug #30050

Merged
merged 2 commits into from Aug 16, 2019
Merged

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented Jul 31, 2019

Bug: country dropdown is clipped by School Info Interstitial modal edge

Fix: set max-height for select element dropdown to 160px

Screen shots

Before:

image (10)

After:

Screen Shot 2019-07-31 at 1 25 46 PM

Gif
country_dropdown

@codecov-io
Copy link

codecov-io commented Aug 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (staging@4c4e114). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             staging   #30050   +/-   ##
==========================================
  Coverage           ?   72.03%           
==========================================
  Files              ?     1368           
  Lines              ?    84453           
  Branches           ?     3397           
==========================================
  Hits               ?    60835           
  Misses             ?    20385           
  Partials           ?     3233
Flag Coverage Δ
#integration 55.37% <ø> (?)
#storybook 56.61% <100%> (?)
#unit 58.15% <100%> (?)
Impacted Files Coverage Δ
apps/src/templates/CountryAutocompleteDropdown.jsx 87.5% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c4e114...a29a889. Read the comment docs.

@nkiruka nkiruka changed the title School Info Completeness: country dropdown bug School Info Interstitial: country dropdown bug Aug 2, 2019
@nkiruka nkiruka mentioned this pull request Aug 2, 2019
@@ -75,6 +75,7 @@ export default class CountryAutocompleteDropdown extends Component {
placeholder={i18n.searchForCountry()}
labelKey="value"
matchPos="start"
maxHeight={160}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copying over my comment from the previous PR:

Ideally we'd let the dropdown expand to fill the screen. Can we clear overflow: auto; on the modal div, or does that cause other problems?

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 set the over-flow to "initial" but it didn't fix the country dropdown bug. This might be due to the overflow property not being a react virtualized select prop (there seems to be a custom option). See react virtualized select

Since maxHeight is react virtualized select prop, setting the dropdown height to be 80% of the total height of the modal is a solution to this bug. Thoughts.

@nkiruka nkiruka requested review from breville and Erin007 and removed request for islemaster and bethanyaconnor August 7, 2019 21:03
@Erin007
Copy link
Contributor

Erin007 commented Aug 8, 2019

How does this change affect other uses of the CountryAutocompleteDropdown? The way you have it now, I think you could pass undefined as a maxHeight to <VirtualizedSelect/>. Please double check that this doesn't cause issues.

Other uses of CountryAutocompleteDropdown: old sign up form (Do we even still need this code?) and census form.

@nkiruka
Copy link
Contributor Author

nkiruka commented Aug 15, 2019

@Erin007 Thanks for the questions. The change does not affect other uses of CountryAutocompleteDropdown

@nkiruka
Copy link
Contributor Author

nkiruka commented Aug 15, 2019

@Erin007 The old_sign_up_form is still in use. From my discussion with Brad & Maddie, the naming of the file is a result of a split test that was conducted previously that was inconclusive. We are still using the “new” flow for Oauth signups, and the “old” flow for email sign-ups. I have an action item to rename the "old_sign_up_form" so it is not confusing.

@nkiruka nkiruka merged commit 2424010 into staging Aug 16, 2019
@nkiruka nkiruka deleted the fix-sii-ui-bug branch August 16, 2019 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants