-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
Update Media Query to Prevent Dropdown Wrapping in Wins Page #6766
Update Media Query to Prevent Dropdown Wrapping in Wins Page #6766
Conversation
…ins page dropdown
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL:
|
@gdkoo Please add to a comment, screenshots of the page at the various break points with the team drop-down opened. The current screenshots before and are of two different things (before: team drop down; after: role drop down). So it's not actually showing the before and after. |
Additional Screenshots of Proposed Changes Of The Website (**Added to PR Body)Breakpoints and dropdown were selected based on which dropdown resulted in wrapping at specified breakpoints Tablet Size 768px (Teams Dropdown): Between Tablet Size and Desktop Size 999px (Role Dropdown): |
@ExperimentsInHonesty Will do. For the desktop size, only the roles dropdown has wrapping, the teams dropdown has no issue. Which means that for each breakpoint, alternating dropdowns are affected. Would you like me to include both dropdowns for each breakpoint, or would my update of before and after images focusing on affected dropdowns for each breakpoint be sufficient? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gdkoo this looks great.
- Pull Request is well-formed and descriptive
- Code change is clean
- Branch checks out in the browser across all screen widths
Can I ask, how did you select the breakpoint max-width: 1024px ? Would it be possible to use one of the existing breakpoints, for example $screen-desktop: 960px?
@roslynwythe: wrapping was an issue up until 999px, so I chose the closest defined screen size defined by the browser (1024px is the desktop size on the browser document inspector). Since 999px is in between ‘screen-desktop’ and ‘desktop-large’ in ‘_sass/variables/_layout.scss’, to set the breakpoint to everything up to ‘screen-desktop-large’ would mean all screen sizes would have the two column drop-down, not just tablets. This seemed like a bigger visual change to the overall layout and I wondered whether this would still fit in as a media query, if the original layout should just be modified instead. I considered if this visual change varied too much from the intended design, then it’d be best to pick a breakpoint that would still leave room for a smaller drop down when screens are at ‘screen-desktop-large’. |
Availability Early Afternoons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jperparas thanks for testing that. Honestly I am unsure; @gdkoo does have a good point about changing the design of the desktop layout, so I would like to ask the product team for input.
@gdkoo I would like to ask product for input on this review but first it would be helpful if you could update the screenshots in the main body of the PR. I see that you responded to Bonnie's request by posting screenshots in a comment, but I'm a bit confused by those because it seems like under the heading "Tablet Size 768px (Teams Dropdown):", the "before" and "after" screenshots might be switched. Also it would be best to avoid confusion if the screenshots were placed in the body of the PR instead of a comment. Thanks
@roslynwythe: Thanks for taking a look. I'll make the changes to the screenshots. |
@ExperimentsInHonesty Please advise - by modifying the value of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gdkoo thank you for adding $screen-desktop-medium: 1024px;
as per the suggestion by @Jperparas, and making use of that breakpoint to resolve the wrapping in the drop downs, as we discussed in the meeting.
Please correct me if I'm wrong, but I think the screenshots in the "Visual Changes" section, under "Between Tablet Size and Desktop Size 999px (Role Dropdown)" are no longer accurate. Would you be willing to post screenshots for "Between Tablet Size and Desktop Size" using a screen size equal or greater than 1024px so that Bonnie can view it in QA?
@roslynwythe If I were to screenshot before and after for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @gdkoo ! This one was a doozy but you stuck in there.
Everything looks good
- Issue linked,
- branch is correct,
- I checked the visuals on my local machine with your most recent commit and it was correct.
Good luck on your next issue!
@roslynwythe Please add an availability and ETA for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @gdkoo - we really want to thank you for your patience and perseverence on this issue.
- the code change is correct and clean
- the PR is well-formed and descriptive
- the changes look good at all screen sizes in the browser
Fixes #6272
What changes did you make?
1000px
$screen-desktop-medium: 1024px
in_sass/variables/_layout.scss
taken from the default 'Laptop' screen size in the document inspector. This changes the dropdown size to two columns when screen is in tablet and desktop, covering all breakpoints where wrapping occurs (up to1000px
), while ensuring that the original layout is still retained for some screen sizes, specifically fromscreen-desktop-medium
toscreen-desktop-xl
(1024px
and above).below-screen-desktop-medium
to cover tablet to desktop sizes.Why did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Breakpoints and dropdown were selected based on which dropdown resulted in wrapping at specified breakpoints
Tablet Size 768px (Teams Dropdown):
Visuals before changes are applied
Visuals after changes are applied:
Between Tablet Size and Desktop Size 999px (Role Dropdown):
Visuals before changes are applied:
Visuals after changes are applied: