Page MenuHomePhabricator

[Cashtab] Add click-outside-area closing effect for NavWrapper using event bubbling
ClosedPublic

Authored by alitayin on Thu, Mar 6, 05:21.

Details

Reviewers
emack
bytesofman
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC767f0d39a1db: [Cashtab] Add click-outside-area closing effect for NavWrapper using event…
Summary

Implemented a click-outside closing mechanism for the settings window using event bubbling instead of useEffect, to maintain consistency with existing code style.
This approach allows the NavWrapper to automatically close when users click outside its boundaries.

Test Plan

Tested on Chrome and Safari browsers across various screen sizes, with the navigation menu closing effect working smoothly.
npm test passes.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
alitayin
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 32656
Build 64800: Build Diffcashtab-tests
Build 64799: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Thu, Mar 6, 05:21
alitayin requested review of this revision.Thu, Mar 6, 05:21

Tail of the build log:

  npm audit fix

Run `npm audit` for details.

> ecash-lib@3.0.0 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

/work/modules/ecash-agora /work/abc-ci-builds/cashtab-tests

added 364 packages, and audited 367 packages in 2s

60 packages are looking for funding
  run `npm fund` for details

6 vulnerabilities (5 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> ecash-agora@2.0.0 build
> tsc && tsc -p ./tsconfig.build.json

/work/cashtab /work/abc-ci-builds/cashtab-tests
npm warn deprecated @humanwhocodes/config-array@0.11.14: Use @eslint/config-array instead
npm warn deprecated @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
npm warn deprecated eslint@8.56.0: This version is no longer supported. Please see https://eslint.org/version-support for other options.

added 1494 packages, and audited 3350 packages in 25s

328 packages are looking for funding
  run `npm fund` for details

9 vulnerabilities (8 moderate, 1 critical)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

> cashtab@3.17.4 build
> node scripts/build.js

Creating an optimized production build...

Treating warnings as errors because process.env.CI = true.
Most CI servers set it automatically.

Failed to compile.

[eslint] 
src/components/App/App.tsx
  Line 5:39:  'useEffect' is defined but never used  @typescript-eslint/no-unused-vars


Build cashtab-tests failed with exit code 1
emack requested changes to this revision.Thu, Mar 6, 06:58

Implemented a click-outside closing mechanism for the settings window

where is this implemented? I'm not seeing event bubbling above, only the removal of the redundant useEffect.

This revision now requires changes to proceed.Thu, Mar 6, 06:58

Please prefix your title with [cashtab] so we now in the review queue what part of the code is changed

alitayin retitled this revision from Add click-outside-area closing effect for NavWrapper using event bubbling to [Cashtab] Add click-outside-area closing effect for NavWrapper using event bubbling.Thu, Mar 6, 08:40

Implemented a click-outside closing mechanism for the settings window

where is this implemented? I'm not seeing event bubbling above, only the removal of the redundant useEffect.

This is a update.
Perhaps my update method wasn't quite right? It created a new sub-diff, this is the initial submission https://reviews.bitcoinabc.org/D17756?id=52941

Implemented a click-outside closing mechanism for the settings window

where is this implemented? I'm not seeing event bubbling above, only the removal of the redundant useEffect.

This is a update.
Perhaps my update method wasn't quite right? It created a new sub-diff, this is the initial submission https://reviews.bitcoinabc.org/D17756?id=52941

You need to squash your commits and run arc diff again so we have all the changes showing up

Squashed commits as requested and removed unnecessary useEffec

emack requested changes to this revision.Fri, Mar 7, 02:34
emack added inline comments.
cashtab/src/components/App/App.tsx
132 ↗(On Diff #52943)

back out and avoid unnecessary linebreaks

389 ↗(On Diff #52943)

what propagation is this actually preventing? You'd normally use this if you're trying to avoid the burger menu closing when clicking on a menu option, which is the opposite of what you're intending with this diff.

In fact, if you back out the changes between line 386 - 391 here, this diff still works as intended. Your onclick logic for CustomApp already achieved the objective.

This revision now requires changes to proceed.Fri, Mar 7, 02:34

and arc diff the next update without the linting bypass pls

Updating D17756: [Cashtab] Add click-outside-area closing effect for NavWrapper using event bubbling

[Cashtab] Add click-outside-area closing effect for NavWrapper using event bubbling

Tested all ok across chrome and firefox.

This revision is now accepted and ready to land.Tue, Mar 11, 04:17

image.png (242×949 px, 36 KB)

looks like this is not based on the latest master or is accidentally a stack

This revision now requires changes to proceed.Tue, Mar 11, 13:17

mb it's not the best git-fu but in my experience it's almost always easier to recreate a diff with a single commit when phab/git raise issues (vs attempt to squash / update all the necessary arcanist stuff...who knows what's in there or what it was originally based on).

it's not a problem once you get into the arcanist/phab workflow and out of the habit of making multiple commits

e.g. for a diff that only changes a few things in one file, you can

git checkout master
git pull
git checkout -b my-recreated-diff

In a browser, navigate to https://reviews.bitcoinabc.org/D17756

Above the only changed file, "View Options -> Show Raw File (Right)"

image.png (434×352 px, 33 KB)

Copy the whole thing and overwrite cashtab/src/components/App/App.tsx with this

git commit -m '[Cashtab] Add click-outside-area closing effect for NavWrapper using event bubbling`
arc diff --draft

wait and confirm that the diff is what you want and that CI builds

then publish for review and abandon the other diff.

otherwise the code is fine, diff is ready to roll with exception of mystery git basis / arc patch issue

An attempt to resolve a missing base commit

This revision is now accepted and ready to land.Tue, Mar 11, 15:10