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 dropdown content position bug #424

Merged
merged 4 commits into from
Oct 31, 2019
Merged

Fix dropdown content position bug #424

merged 4 commits into from
Oct 31, 2019

Conversation

nabeliwo
Copy link
Member

problem

image
☝️ Can no longer scroll upward

Under the following conditions, DropdownContent display position will be strange as shown in the image above.

  • The sum of the position at the bottom of DropdownTrigger and the height of DropdownContent exceeds innerHeight
  • The number obtained by subtracting DropdownContent from the upper position of DropdownTrigger is less than 0

ideal

For the conditions listed above, the DropdownContent is displayed at the appropriate size below the DropdownTrigger and is scrollable.

image

what I did

I made it ideal!!!!!!

@nabeliwo nabeliwo requested a review from a team as a code owner October 30, 2019 10:03
@reg-suit
Copy link

reg-suit bot commented Oct 30, 2019

✨✨ That's perfect, there is no visual difference! ✨✨

Check out the report here.

@ghost ghost requested review from cidermitaina and removed request for a team October 30, 2019 10:03
@nabeliwo nabeliwo requested review from im36-123 and removed request for cidermitaina October 30, 2019 10:03
@nabeliwo
Copy link
Member Author

@im36-123 Please review!!

Copy link
Contributor

@im36-123 im36-123 left a comment

Choose a reason for hiding this comment

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

Good work 🎉
I left a comment. Please check 🙏

Comment on lines 40 to 43
} else {
position.top = `${scroll.top + triggerRect.top - contentSize.height}px`
const paddingBottom = 10
contentBox.top = `${scroll.top + triggerRect.bottom}px`
contentBox.maxHeight = `${windowSize.height - triggerRect.bottom - paddingBottom}px`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worry about case of the window bottom space is not enough.
This is un extreme example, but in case the below, the user hard to reads context in content.

image

So I think good solution is to determine the display position of the content based on whether the button is located above or below the window.

@nabeliwo
Copy link
Member Author

@im36-123 Thank you for review.

Fixed.
It's still hard to see, but it's better than before. I think that there is no problem with this correspondence because it is an edge case.

image

@im36-123 im36-123 self-requested a review October 31, 2019 06:14
Copy link
Contributor

@im36-123 im36-123 left a comment

Choose a reason for hiding this comment

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

Thank you great contribution !!
Great bug fix 🎉🎉🎉

@nabeliwo nabeliwo merged commit c00dc0a into features/ttmz Oct 31, 2019
@nabeliwo nabeliwo deleted the fix-dropdown branch October 31, 2019 06:24
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

2 participants