Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Patch: "Reloading the registration page should warn about data loss" #8377

Merged
merged 15 commits into from
Apr 29, 2022
Merged

Patch: "Reloading the registration page should warn about data loss" #8377

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ac39d61
warn of data loss on reloading registration page
yaya-usman Apr 20, 2022
3c47141
update: warn on loss of data on refresh
yaya-usman Apr 20, 2022
e37af06
Merge branch 'develop' of https://github.com/yaya-usman/matrix-react-…
yaya-usman Apr 20, 2022
2c9712b
update: unloadCallback made private
yaya-usman Apr 20, 2022
b29c417
proper event annotation
yaya-usman Apr 21, 2022
872d0e0
resolve conflict
yaya-usman Apr 21, 2022
9fe77a8
update: popup the dialog warning at a later stage of registration
yaya-usman Apr 21, 2022
8b0dca2
updated branch
yaya-usman Apr 24, 2022
9b92c90
show warn dialog only at a later stage of registration
yaya-usman Apr 24, 2022
48636c6
Merge branch 'develop' of https://github.com/yaya-usman/matrix-react-…
yaya-usman Apr 25, 2022
584d54e
updated fix
yaya-usman Apr 25, 2022
8ed065f
resolve conflict
yaya-usman Apr 27, 2022
75c3627
updated warn dialog logic
yaya-usman Apr 27, 2022
71e9932
Merge branch 'develop' of https://github.com/yaya-usman/matrix-react-…
yaya-usman Apr 29, 2022
736eae1
updated fix
yaya-usman Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/components/views/auth/CaptchaForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,21 @@ export default class CaptchaForm extends React.Component<ICaptchaFormProps, ICap
);
this.recaptchaContainer.current.appendChild(scriptTag);
}
//triggers a confirmation dialog for data loss before page unloads/refreshes
window.addEventListener("beforeunload", this.unloadCallback);
}

componentWillUnmount() {
this.resetRecaptcha();
window.removeEventListener("beforeunload", this.unloadCallback);
}

private unloadCallback = (event: BeforeUnloadEvent) => {
event.preventDefault();
event.returnValue = "";
return "";
};

// Borrowed directly from: https://github.com/codeep/react-recaptcha-google/commit/e118fa5670fa268426969323b2e7fe77698376ba
private isRecaptchaReady(): boolean {
return typeof window !== "undefined" &&
Expand Down
12 changes: 12 additions & 0 deletions src/components/views/auth/InteractiveAuthEntryComponents.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,20 @@ export class TermsAuthEntry extends React.Component<ITermsAuthEntryProps, ITerms

componentDidMount() {
Copy link
Member

Choose a reason for hiding this comment

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

This component can be used outside the context of registration, lets please not scope creep

This component already also renders CaptchaForm so two listeners will be active when the captcha form is active

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it's not working if i only placed the listener in one of the other, so i had to place it in both, i believe they are seperate confirmation entity. you can check to confirm on your browser

Copy link
Member

Choose a reason for hiding this comment

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

It should be possible to keep the listener in a single location in the Registration component, and then add and remove it on the fly as registration moves through the different stages using componentDidUpdate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure i really understand this clearly, as requested by @t3chguy the warning should only be available at a later stage of the registration which is fine but according to your suggestion now, is it possible to actually detect if it has moved from one stage to another since all the stages are just having one route /register?.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the Registration component holds state that determines which stage of the registration process the user is on, so we should be able to detect in that component when the user has moved past the first stage

Copy link
Contributor Author

@yaya-usman yaya-usman Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I need a bit of your help here, currently the state variable i think keeps track of the stages is flow which is an array of object containing a property named stages which is also an array,the issue here is that i tried using componentDidUpdate and log to the console to check if there is a state change btwn previous and current so i can leverage on that but unfortunately there isn't, also immediately i hit the register button, it stops logging the current and prev values and rather logs the following instead, which i couldn't find anywhere in the project

console

This is when it still logs the curr and prev:

console2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also lastly, in my opinion including the listeners in both components as i have done previously doesn't hurt atall because throughout the entire application it's only the Auth section that makes use of this components, even if it were to be used by another component, it still serves it's purpose of not allowing user refresh without warning them of a data loss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nonetheless, i'll appreciate if you guys can point me in the right direction.

Copy link
Member

@t3chguy t3chguy Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think just conditioning on this.state.doingUIAuth should be fine

it's only the Auth section that makes use of this components

Auth yes, but not Registration. The issue was about Registration, not changing password/deleting devices/deactivating account which can all also run the UIA code. Ideally lets keep to the issue to not scope creep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok boss, thanks... Will try to work things out as you have suggested.

this.props.onPhaseChange(DEFAULT_PHASE);
//triggers a confirmation dialog for data loss before page unloads/refreshes
window.addEventListener("beforeunload", this.unloadCallback);
}

componentWillUnmount() {
window.removeEventListener("beforeunload", this.unloadCallback);
}

private unloadCallback = (event: BeforeUnloadEvent) => {
event.preventDefault();
event.returnValue = "";
return "";
};

private togglePolicy(policyId: string) {
const newToggles = {};
for (const policy of this.state.policies) {
Expand Down