Skip to content

Conversation

CooperHash
Copy link

@CooperHash CooperHash commented Mar 17, 2024

ref: #668

background

When we set the popupOffset, we expect all submenus to have an offset from the menu item. However, when the window width is small, the menu overflows. The overflow, represented by ... as an overflow placeholder, does not have a submenu offset.

It is necessary to fix it

before fix:

menu.mov

after fix:

menu.mov

internalPopupClose={len === 0}
popupClassName={overflowedIndicatorPopupClassName}
popupOffset={overflowPopupOffset || [10, 15]}
>
Copy link
Member

Choose a reason for hiding this comment

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

Why is the default value this?

Copy link
Author

Choose a reason for hiding this comment

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

i refer rc-menu docs's demo
if not, what value is appropriate

Copy link
Member

Choose a reason for hiding this comment

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

@zombieJ @MadCcc @afc163 Pls take a look.

@yoyo837
Copy link
Member

yoyo837 commented Mar 18, 2024

And, some new test case is needed.

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (4ae012f) to head (57b5938).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #690   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          27       27           
  Lines         721      722    +1     
  Branches      196      199    +3     
=======================================
+ Hits          718      719    +1     
  Misses          3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

forceSubMenuRender?: boolean;
triggerSubMenuAction?: TriggerSubMenuAction;
builtinPlacements?: BuiltinPlacements;
overflowPopupOffset?: number[]
Copy link
Member

Choose a reason for hiding this comment

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

From a functional and naming standard, should this be provided? @MadCcc

Copy link
Author

Choose a reason for hiding this comment

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

i refer from this

popupOffset?: number[];

@CooperHash
Copy link
Author

And, some new test case is needed.

i have no idea how to mock overflow

@alekseykurylev
Copy link

@zombieJ @yoyo837 When should this PR be expected in the release?

@CooperHash
Copy link
Author

@zombieJ @yoyo837 When should this PR be expected in the release?

I haven't written the test yet, so you'll need to wait for me to finish writing it before they can review it.

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.

4 participants