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 Searchbar Dropdown Menu #2567

Merged
merged 4 commits into from
Aug 11, 2024
Merged

Conversation

gerteck
Copy link
Contributor

@gerteck gerteck commented Aug 9, 2024

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:

Fixes #2566

To fix the skew of the initial positioning of the dropdown-menu, changes the use of d-none (display:none !important) which removes the element from the DOM to visibility:hidden which instead allows it to persist in DOM even when hidden, such that the dropdown menu position does not get skewed.

Anything you'd like to highlight/discuss:

Another solution I came up with was to add a div container with relative positioning as a parent to the dropdown-menu, as such:

+   <div class="relative-position">
      <ul ref="dropdown" :class="dropdownMenuClasses">
        <li
          v-for="(item, index) in items"
          :key="index"
          :class="{ 'table-active': isActive(index) }"
        >
          <a
            class="dropdown-item"
            @mousedown.prevent="hit"
            @mousemove="setActive(index)"
          >
            <searchbar-page-item :item="item" :value="value" />
          </a>
        </li>
      </ul>
+   </div>

It could be due to the way Popper.js works, as it helps to retain the DOM structure as if the dropdown-menu was there even when it is removed due to display:none. However, I thought it might be a hacky solution that could cause confusion later on, making it less ideal, although less invasive and perhaps worth considering.

Testing instructions:

Test as per #2566:

We can recreate this issue on MB's user guide page, page on search bar.
Type in a search term (e.g. 'search bar'), do not do any mouse scroll.
Dropdown menu is rendered, but cannot be seen.
Do a mouse scroll, and the dropdown menu repositions correctly.
To recreate, just click out, click back in, and type another search term
Do a mouse scroll, and the dropdown menu reappears

Proposed commit message: (wrap lines at 72 characters)

Fix Searchbar dropdown-menu positioning

Fix initial dropdown-menu positioning by replacing d-none
with visibility:hidden. Popper.js cannot calculate the position of
elements removed from the DOM with display: none. Let's
replace d-none with visibility:hidden, keeping the element
in the DOM, allowing Popper.js to correctly calculate its position.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the through investigation and nice catch :)

Copy link
Contributor

@Tim-Siu Tim-Siu 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 for your thorough investiagation! The solution of the PR and the alternative solution both look pretty elegant.

I agree that the method in this PR may cause less confusion, as future maintainer may accidently delete the container.

Overral this is a sound PR. It looks good to me.

@Tim-Siu Tim-Siu merged commit 70ba5e5 into MarkBind:master Aug 11, 2024
9 checks passed
@github-actions github-actions bot added the r.Patch Version resolver: increment by 0.0.1 label Aug 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r.Patch Version resolver: increment by 0.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skewed initial positioning of Search Bar's Dropdown menu
3 participants