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

IBX-186: The top button of side menus gets hidden at the bottom of the page #1763

Conversation

GrabowskiM
Copy link
Contributor

Question Answer
Tickets IBX-186
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

Copy link
Member

@dew326 dew326 left a comment

Choose a reason for hiding this comment

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

I think we can do better if it comes to naming. I suggested some names, feel free to enhance them.

The target branch should be different.


container.style.height = `${containerHeight}px`;
};
const setHeightOnScrollAndResize = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const setHeightOnScrollAndResize = () => {
const setContainerHeightTimeout = () => {

?

const setHeightOnScrollAndResize = () => {
clearTimeout(resizeAndScrollTimeout);

resizeAndScrollTimeout = setTimeout(setContainerHeight, RESIZE_AND_SCROLL_TIMEOUT);
Copy link
Member

@dew326 dew326 May 26, 2021

Choose a reason for hiding this comment

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

Suggested change
resizeAndScrollTimeout = setTimeout(setContainerHeight, RESIZE_AND_SCROLL_TIMEOUT);
containerHeightTimeout = setTimeout(setContainerHeight, RESIZE_AND_SCROLL_TIMEOUT);

?


resizeAndScrollTimeout = setTimeout(setContainerHeight, RESIZE_AND_SCROLL_TIMEOUT);
};
const setHeightOnResizeAndScroll = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const setHeightOnResizeAndScroll = () => {
const addListeners = () => {

?

global.addEventListener('scroll', setHeightOnScrollAndResize, false);
global.addEventListener('resize', setHeightOnScrollAndResize, false);
};
const resetHeightOnResizeAndScroll = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const resetHeightOnResizeAndScroll = () => {
const removeListeners = () => {

?

@GrabowskiM GrabowskiM closed this May 27, 2021
@GrabowskiM GrabowskiM force-pushed the IBX-186-the-top-button-of-side-menus-gets-hidden-at-the-bottom-of-the-page branch from 4f48689 to 8b6a5ee Compare May 27, 2021 10:40
@GrabowskiM GrabowskiM reopened this May 27, 2021
@GrabowskiM GrabowskiM changed the base branch from master to 2.1 May 27, 2021 10:43
@GrabowskiM GrabowskiM requested a review from dew326 May 27, 2021 10:44
@@ -33,6 +57,7 @@
}

doc.body.removeEventListener('click', detectClickOutside, false);
resetHeightOnResizeAndScroll();
Copy link
Member

Choose a reason for hiding this comment

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

Error function not found?

@GrabowskiM GrabowskiM requested a review from dew326 May 27, 2021 11:00
@GrabowskiM GrabowskiM force-pushed the IBX-186-the-top-button-of-side-menus-gets-hidden-at-the-bottom-of-the-page branch from c52c941 to e27847a Compare May 28, 2021 13:09
@GrabowskiM GrabowskiM changed the base branch from 2.1 to 2.2 May 28, 2021 13:09
Copy link
Contributor

@kacper-wieczorek-ibexa kacper-wieczorek-ibexa left a comment

Choose a reason for hiding this comment

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

QA - Approved. Tested on 3.2.7-dev experience, 3.3.3-dev experience.

@lserwatka
Copy link
Member

You can merge it up.

@lserwatka lserwatka deleted the IBX-186-the-top-button-of-side-menus-gets-hidden-at-the-bottom-of-the-page branch May 29, 2021 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants