Skip to content

refactor(context-menu): scroll into view if needed #10545

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

Closed
wants to merge 2 commits into from

Conversation

aboqasem
Copy link

@aboqasem aboqasem commented Jun 9, 2022

Description

Scroll context menus into view when needed.

Related Issue(s)

Fixes #9326

How to test

scrollIntoViewIfNeeded.mov

Release Notes

scroll context menus into view when needed

Documentation

@aboqasem aboqasem requested a review from a team June 9, 2022 05:58
@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 9, 2022

Thanks! ♥️ Flagging this for review too.

@stale
Copy link

stale bot commented Jun 19, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jun 19, 2022
@laushinka laushinka added the do-not-merge/cla-pending CLA has not been signed label Jun 20, 2022
@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Jun 20, 2022
@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 27, 2022

Thanks for contributing, @aboqasem! 🍊

Ideally, we would detect when there isn't enough space in the viewport and position the dropdown above the button when needed, without requiring any scrolling. Do you think this could be easy to change here? Otherwise, we could leave this as is for now as it seems like an improvement and open a follow-up issue to properly resolve this. ❓

You'll also need to sign a Contributor License Agreement (CLA)[1] once before merging your first contribution. Looping in @meysholdt to reach out about the CLA. 🏓

@aboqasem
Copy link
Author

aboqasem commented Jun 30, 2022

Thank you for your review!

The issue states that scrolling is a possible solution 😅. Regarding the solution you've mentioned, how could the size of the menu be determined without viewing it? Is it by having fixed numbers representing menu item heights on each screen breakpoint and then using it with the number of menu items to determine whether there is available space or not? Or is there something else?

For the CLA, I think I must commit with my email that signed the CLA, not the Github noreply email right?

@stale
Copy link

stale bot commented Jul 10, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jul 10, 2022
@aboqasem aboqasem force-pushed the improve/dropdown-visibilty branch from 92e7162 to fc06bcd Compare July 10, 2022 13:58
@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Jul 10, 2022
@meysholdt
Copy link
Member

hi @aboqasem and thank you for your contributions! Since I can't find a CLA of you on file, can you please sign one as requested here? thank you!

@meysholdt
Copy link
Member

Thx for signing the CLA!

@meysholdt meysholdt added cla: accepted ✔️ and removed do-not-merge/cla-pending CLA has not been signed labels Jul 26, 2022
@jankeromnes
Copy link
Contributor

jankeromnes commented Aug 1, 2022

/werft run

👍 started the job as gitpod-build-improve-dropdown-visibilty-fork.0
(with .werft/ from main)

@jankeromnes
Copy link
Contributor

jankeromnes commented Aug 1, 2022

Hi @aboqasem! Many thanks for implementing this fix. 💯 This looks good to me, but I'd love to test it before approving.

In order to test it, I'd need a full build of this Pull Request, however I notice that the build currently fails -- the failure looks unrelated to your changes, but could be caused by your PR being based on an old commit.

Could you please go through the following steps in order to update this PR?

  1. Open this PR in Gitpod, e.g. by clicking this link: https://gitpod.io/#https://github.com/gitpod-io/gitpod/pull/10545
  2. In the terminal, run these commands:
git pull --rebase upstream main

git push -f origin

This should fix the build and allow me to review and approve your PR. 🙂

@aboqasem aboqasem force-pushed the improve/dropdown-visibilty branch from fc06bcd to a6c8ede Compare August 6, 2022 09:54
@aboqasem
Copy link
Author

aboqasem commented Aug 6, 2022

Hi @jankeromnes! Thank you for the review. I have updated the PR and looking forward to it being merged!

@laushinka
Copy link
Contributor

laushinka commented Aug 15, 2022

@jankeromnes Would you like to review it again? Also happy to take it over 🙏🏽

@stale
Copy link

stale bot commented Sep 8, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Sep 8, 2022
@stale stale bot closed this Sep 20, 2022
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.

Make the the dropdown visible when expanding beyond the viewport
6 participants