-
Notifications
You must be signed in to change notification settings - Fork 22
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
#8868: slice 4: re-write EllipsisMenu using the react menu library #8887
#8868: slice 4: re-write EllipsisMenu using the react menu library #8887
Conversation
…8868-slice-4-re-write-actionmenutsx-using-the-react-menu-library
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8887 +/- ##
==========================================
+ Coverage 74.24% 74.47% +0.22%
==========================================
Files 1332 1339 +7
Lines 40817 41205 +388
Branches 7634 7702 +68
==========================================
+ Hits 30306 30688 +382
- Misses 10511 10517 +6 ☔ View full report in Codecov by Sentry. |
Playwright test resultsDetails Open report ↗︎ Flaky testsedge › tests/regressions/welcomeStarterBricks.spec.ts › #8740: can view the starter mods on the pixiebrix.com/welcome page Skipped testschrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor |
.toggle:global(.dropdown-toggle)::after { | ||
display: none; | ||
} | ||
.button { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most widely used styling (Integration Card, active mods)
@@ -85,104 +68,64 @@ type EllipsisMenuProps = { | |||
* The boundary element for the dropdown menu popup | |||
* @see DropdownMenuProps.popperConfig | |||
*/ | |||
menuBoundary?: Element; | |||
boundingBoxRef?: MutableRefObject<HTMLElement | null>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same behavior as the menuBoundary, but different implementation
<Tab.Content className={dataPanelStyles.tabContent}> | ||
<Tab.Content | ||
className={dataPanelStyles.tabContent} | ||
ref={(element: HTMLElement) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice refactor using ref.
const [activeElement, setActiveElement] = useReduxState( | ||
selectActiveBuilderPreviewElement, | ||
actions.setActiveBuilderPreviewElement, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's at least one hook that gets activeElement
from redux using useSelector. Using useState here was causing the store not to be updated, leading to an NPE when running one of the tests,
I understand why the test was failing, I don't understand how it ever passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the tests just aren't covering behavior relevant to those hooks? 🤷♂️
…e-react-menu-library
…8868-slice-4-re-write-actionmenutsx-using-the-react-menu-library
…8868-slice-4-re-write-actionmenutsx-using-the-react-menu-library
When the PR is merged, the first loom link found on this PR will be posted to |
What does this PR do?
Discussion
Remaining Work
Demo
Extension
App
Future Work
Checklist
@szhsin/react-menu
: MITFor more information on our expectations for the PR process, see the
code review principles doc