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 degree and employment form fields #5034

Merged
merged 1 commit into from
Sep 17, 2021
Merged

fix degree and employment form fields #5034

merged 1 commit into from
Sep 17, 2021

Conversation

umarmughal824
Copy link
Contributor

@umarmughal824 umarmughal824 commented Sep 2, 2021

Pre-Flight checklist

  • Screenshots and design review for any changes that affect layout or styling
    • Desktop screenshots
    • Mobile width screenshots
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

fixes #5019

What's this PR do?

Can't select some countries in pulldowns on phone in portrait mode

How should this be manually tested?

Just see the country field on the profile page by adding degree and employment

Screenshots (if appropriate)

MOBILE

Screenshot 2021-09-10 at 14 40 32

TABLET

Screenshot 2021-09-10 at 14 40 46

DESKTOP

Screenshot 2021-09-10 at 14 40 55

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2021

Codecov Report

Merging #5034 (4ef87c3) into master (cf3ffed) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 4ef87c3 differs from pull request most recent head d7fc92e. Consider uploading reports for the commit d7fc92e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5034      +/-   ##
==========================================
- Coverage   94.05%   94.04%   -0.01%     
==========================================
  Files         500      500              
  Lines       22972    22972              
  Branches      932      932              
==========================================
- Hits        21607    21605       -2     
- Misses       1267     1268       +1     
- Partials       98       99       +1     
Impacted Files Coverage Δ
static/js/components/EducationForm.js 96.64% <ø> (ø)
static/js/components/EmploymentForm.js 96.61% <ø> (ø)
courses/utils.py 95.12% <0.00%> (-4.88%) ⬇️

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 cf3ffed...d7fc92e. Read the comment docs.

@briangrossman
Copy link
Contributor

@umarmughal824 - Visually, the screenshots you shared look great! I think the longest country is:
Ascension and Tristan Da Cunha, Saint Helena, it would be good to check that one, in particular.

@umarmughal824
Copy link
Contributor Author

umarmughal824 commented Sep 3, 2021

@briangrossman this is how it looks like

Screenshot 2021-09-03 at 12 30 47

Screenshot 2021-09-03 at 12 30 56

@briangrossman
Copy link
Contributor

@briangrossman this is how it looks like

Screenshot 2021-09-03 at 12 30 47

Great. Thanks!!! 👍

@umarmughal824
Copy link
Contributor Author

@annagav could you please review it?

@annagav annagav self-assigned this Sep 8, 2021
Copy link
Contributor

@annagav annagav left a comment

Choose a reason for hiding this comment

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

Could we achieve the same result on mobile view without also modifying the full desktop view of the card?

@umarmughal824
Copy link
Contributor Author

@briangrossman do you think should we should update the desktop screens or should we keep them intact as per the attached screenshots.

@briangrossman
Copy link
Contributor

@umarmughal824

If possible, I think it would be better to have this PR only only affect the mobile view. Ideally, the desktop would continue to look like it currently does on the production site, rather than modifying it:

image

@annagav
Copy link
Contributor

annagav commented Sep 9, 2021

@briangrossman this is how it used to be before:

Screen Shot 2021-09-09 at 9 44 01 PM

and here is in this PR:

Screen Shot 2021-09-09 at 9 43 00 PM

@umarmughal824 umarmughal824 removed the request for review from arslanashraf7 September 10, 2021 09:08
@umarmughal824
Copy link
Contributor Author

umarmughal824 commented Sep 10, 2021

@briangrossman @annagav I have updated the screenshots see in the PR description. I have updated the tablet version too as it seems problematic.

@umarmughal824 umarmughal824 changed the title fix degree and employment form fields on mobile level fix degree and employment form fields Sep 10, 2021
@briangrossman
Copy link
Contributor

@briangrossman @annagav I have updated the screenshots see in the PR description. I have updated the tablet version too as it seems problematic.

@umarmughal824 Thanks! I see what you're saying with the wrapping of the city field for the tablet version. Thanks for sharing that.

I think it's fine the way you have it now. The most important thing is that you've fixed the options in the pulldown so they don't overlap.

Copy link
Contributor

@annagav annagav left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@umarmughal824 umarmughal824 merged commit d2e389c into master Sep 17, 2021
@umarmughal824 umarmughal824 deleted the umar/fix-5019 branch September 17, 2021 12:56
This was referenced Sep 17, 2021
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.

Can't select some countries in pulldowns on phone in portrait mode
5 participants