Improved /account page blocking#2255
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2255 +/- ##
========================================
Coverage 43.22% 43.22%
========================================
Files 603 603
Lines 25775 25793 +18
Branches 7662 8577 +915
========================================
+ Hits 11141 11149 +8
- Misses 14585 14587 +2
- Partials 49 57 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sjd210
left a comment
sjd210
left a comment
There was a problem hiding this comment.
Also, an interesting quirk that I don't think matters: Changing data in one tab and then navigating away to another tab with a hash anchor will still cause the "You have unsaved changes on this page" dialog on trying to click away, despite the user now being on a different page without that data.
We don't surface the hash anchors on the page, so it would have to be done intentionally, and we still fail safely - so if a user puts themself in that position, so be it.
| } | ||
| sidebar={siteSpecific( | ||
| <MyAccountSidebar editingOtherUser={editingOtherUser} activeTab={activeTab} setActiveTab={setActiveTab}/>, | ||
| <MyAccountSidebar editingOtherUser={editingOtherUser} activeTab={activeTab} setActiveTab={safelyChangeTab}/>, |
There was a problem hiding this comment.
This safelyChangeTabs is firing twice, so whenever the confirm dialog is cancelled - it immediately comes straight back up.
It turns out that this is kinda a pre-existing issue with StyledTabPicker (although one that doesn't matter for most instances, since duplicating a navigate or setState makes no difference).
When we use an
onClick/onKeyboardDown system for StyledTabPicker like MyAccountSidebar does, the onClick gets set to the above Label AND activates the Input within so sends two events.
Some StyledTabPicker instances use onInputChange directly, which avoids this - so we should probably either change all instances to that site-wide OR we can protect against the issue within StyledTabPicker by passing onClick as a prop directly to the Input and not to the Label (as is done via {...rest}). Either works 🤷♀️
There was a problem hiding this comment.
I think this is almost right – but the input activating isn't doing anything as it doesn't have an event handler. The Label element is, however, repeating the onClick method because of the input; this post describes how onClick is hitting both the label and the input, but the input's one is getting propagated up to the label so that one fires twice. Your fix works because onInputChange is explicitly not applied to the Label but is to the Input.
To prove this, a third alternative solution (not suggesting we use this, just as proof of concept) is to stop the propagation of the onClick and onChange events inside the input:
<Input ... onClick={(e) => e.stopPropagation()} onChange={(e) => {e.stopPropagation(); onInputChange?.(e);}}... />I don't really know if we should be running event handlers in the input or the label – as the post describes, events that hit the label are also sent to the input if they are tied, but if they are nested events will also propagate from the input to the label, so it seems arbitrary which we choose. Perhaps the input is the more logical place for them; they shouldn't fire twice if existing there, I think.
There was a problem hiding this comment.
I've made a cleanup branch targeting this one here; it's quite a large set of changes so figured it would be best reviewed separately? Do let me know your thoughts.
There was a problem hiding this comment.
That Stack Overflow post is also how I found out what the issue was. I kinda just... didn't fully explain it while reviewing, just giving the gist (although I certainly could've linked it too) 🙃 My bad
Reviewing the cleanup branch now
An issue with `onChange`, as highlighted by a failing VRT, is that selecting an item that was previously checked does not do anything, unlike `onClick`. This set of changes therefore re-allows `onClick` and `onKeyDown` as props on the picker, but requires `onChange` such that mixing usages of both must be carefully considered.
…hange-cleanup StyledTabPicker onChange cleanup
2 participants
We have several issues in the past regarding the account page preventing saving owing to a field being invalid in a different tab.
For example, you could:
Admittedly, an option becoming invalid is unlikely, but accidental changes are not – it wouldn't be obvious that e.g. a name change has occurred if you weren't on the tab.
As a solution to these, we would instead prefer that there be a warning if you attempt to switch tabs with unsaved changes. If you proceed regardless, the tab states should all reset, so that if you make adjustments to another tab, the first set of changes cannot block the changes.