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

1806 - ids-toolbar misposition in header overflow menu #2276

Merged
merged 6 commits into from
Jun 17, 2024

Conversation

clayinfor
Copy link
Collaborator

Explain the details for making this change. What existing problem does the pull request solve?

Fix so that menus and submenu popup positions are correct when using IdsToolbar inside IdsAppMenu (and IdsHeader)

Related github/jira issue (required):

Closes #1806

Steps necessary to review your pull request (required):

  1. Check out this branch and run: nvm use && npm install && npm run start
  2. Then go to: http://localhost:4300/ids-toolbar/in-header.html
  3. Click the hamburger button on the left of the toolbar title. This should open up the application side-panel menu.
  4. Click on the more-actions button (the 3 dots) on the right side of the toolbar.
  5. The popup menu should appear properly positioned, and a horizontal scrollbar should not appear.
  6. Hover over the "More Options" submenu in the more actions menu, and ensure the submenu appears.

Included in this Pull Request:

  • Some documentation for the feature.
  • A test for the bug or feature.
  • A note to the change log.

@clayinfor clayinfor requested a review from a team as a code owner May 1, 2024 11:12
Copy link
Member

@tmcconechy tmcconechy left a comment

Choose a reason for hiding this comment

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

@clayinfor the one issue i see is that its loosing the highlight.

Screenshot 2024-05-01 at 10 24 37 AM

The highlight used to stay on the both menu items similar to this:

Screenshot 2024-05-01 at 10 24 21 AM

Copy link
Contributor

@jmacaluso711 jmacaluso711 left a comment

Choose a reason for hiding this comment

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

Looks good. Seeing one issue. It might be related to the popup-menu component itself, but on smaller screens it is getting cut off.

Screenshot 2024-05-01 at 10 52 52 AM

Copy link
Member

@tmcconechy tmcconechy left a comment

Choose a reason for hiding this comment

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

Marking as request changes for the above

@tmcconechy tmcconechy added the status: wip 🚧 Work in Progress (Ignore for now) label May 6, 2024
if (this.containingElem?.classList?.contains('app-menu-is-open')) {
const appMenu = this.containingElem?.querySelector('.app-menu');
const appMenuRect = appMenu?.getBoundingClientRect();
if (navigator.userAgent.indexOf('Firefox') === -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this cause a warning? May also want to sync main here as this PR is now pretty old

@tmcconechy
Copy link
Member

@clayinfor any updates on this one? Been sitting a long time

@tmcconechy
Copy link
Member

@clayinfor any updates on this one? Been sitting a long time. Shouldnt need too much to finish it?

…osition-in-header-overflow-menu

Conflicts:
doc/CHANGELOG.md
src/components/ids-popup-menu/ids-popup-menu.ts
@clayinfor
Copy link
Collaborator Author

hey @tmcconechy, let's merge this, and create a new task to address the remaining issues.

@tmcconechy
Copy link
Member

@clayinfor ok great. Can you make the issue as im not sure i know whats left?

@clayinfor
Copy link
Collaborator Author

Ok @tmcconechy , I used John's comment to create an issue here.

@tmcconechy tmcconechy merged commit 69f3dac into main Jun 17, 2024
4 checks passed
@tmcconechy tmcconechy deleted the 1806-ids-toolbar-misposition-in-header-overflow-menu branch June 17, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip 🚧 Work in Progress (Ignore for now)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IdsToolbar: Toolbar in IDSHeader with an IDSAppMenu will misposition the overflow menu
3 participants