Fix dependent tag second-level row stuck faded after selecting first-level tag#94756
Fix dependent tag second-level row stuck faded after selecting first-level tag#94756MelvinBot wants to merge 2 commits into
Conversation
…hlight style Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| // The animated highlight style is always applied (never swapped on `highlighted`) so the entry fade is never | ||
| // detached from the node mid-animation. When `highlighted` is false the hook returns a static opacity: 1, so | ||
| // this is inert for non-highlighted rows. See https://github.com/Expensify/App/issues/94005. | ||
| outerWrapperStyle={[outerWrapperStyle, animatedHighlightStyle]} |
There was a problem hiding this comment.
@MelvinBot No need for a comment here and instead of rendering a MenuItem, let's render MenuItemWithTopDescription here.
There was a problem hiding this comment.
@MelvinBot No need for a comment here and instead of rendering a MenuItem, let's render MenuItemWithTopDescription here.
There was a problem hiding this comment.
Done in 7f5ec07. Now renders MenuItemWithTopDescription (which already sets shouldShowBasicTitle/shouldShowDescriptionOnTop) and removed the comment.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppios.mp4iOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safariweb.mp4 |
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
@MelvinBot i've done testing and everything looks good. Please complete the author checklist and mark this PR as ready |
|
@bernhardoj Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@bernhardoj I think Melvin swallowed some checlist items |
|
@MelvinBot remove the Screenshot/Videos section and replace the PR Author Checklist with below checklist checklistPR Author Checklist
|
|
@MelvinBot remove the unchecked checklist. It's not part of the checklist. |
|
@MelvinBot replace the Screenshot/Videos section with below videosScreenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppios.mp4iOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safariweb.mp4 |
Explanation of Change
When a dependent (multi-level) tag's first level is selected, the newly-revealed second-level tag row (e.g.
Regionafter selectingState) appeared empty/faded on iOS and Android until the user tapped it.Root cause:
MenuItemWithTopDescriptionapplied the entry-fade animation style only while itshighlightedprop wastrue(outerWrapperStyle={highlighted ? highlightedOuterWrapperStyle : outerWrapperStyle}). For a freshly-revealed dependent tag row,highlightedflipstrue → falseon the very next render — mid-animation — which detaches the animated highlight style from the native node before the fade'swithTimingcompletes. The shared value keeps animating to1on the UI thread, but nothing reads it anymore, so the row freezes at a partial opacity. Web was unaffected because it re-applies the flattened style on every React commit.Fix: Extracted the highlight into a dedicated
HighlightableMenuItemWithTopDescriptionthat owns theuseAnimatedHighlightStylehook and always applies the animated style (no longer gated onhighlighted), so the style stays attached for the entire fade. Whenhighlightedisfalse, the hook returns a staticopacity: 1with no pulse (initialNonRepeatableProgressValue = 1), so the always-applied style is inert for non-highlighted rows.MenuItemWithTopDescriptionis reverted to a plain pass-through, leaving a single highlight implementation.Migrated the three remaining
MenuItemWithTopDescriptioncall sites that passhighlighted:MoneyRequestConfirmationList/sections/TagFields.tsx(the reported bug)pages/iou/SplitExpenseEditPage.tsx(same dependent-tag pattern)ReportActionItem/MoneyRequestView.tsxThe
WorkspaceInitialPage/DomainInitialPagecall sites already use the separateHighlightableMenuItemcomponent (which already applies its highlight style unconditionally), so they needed no change.Fixed Issues
$ #94005
PROPOSAL: #94005 (comment)
Tests
Offline tests
Same as Tests.
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionvideos
Screenshots/Videos
Android: HybridApp
android.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: HybridApp
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4
/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)