-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add localstorage saving to meeting report #4627
base: master
Are you sure you want to change the base?
Conversation
fadc15c
to
78e9126
Compare
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.
LGTM!
@@ -451,6 +498,26 @@ const MeetingEditor = () => { | |||
)} | |||
</ConfirmModal> | |||
)} | |||
{isEditPage && | |||
canDelete && |
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.
Why do you need this line?
@@ -105,6 +105,8 @@ type MeetingEditorParams = { | |||
}; | |||
const MeetingEditor = () => { | |||
const { meetingId } = useParams<MeetingEditorParams>(); | |||
const [formDirtyFlag, setFormDirtyFlag] = useState(0); |
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.
Do you have any reasoning behind this naming? Didn't find it too intuitive
Also the method of refreshing the form is still kinda hacky - so might be nice with a comment about why this is needed.
const [formDirtyFlag, setFormDirtyFlag] = useState(0); | |
const [reportKey, setReportKey] = useState(0); // Needed to refresh the form component after loading the report content from localStorage |
Now I'm just throwing out ideas, but I'm assuming this prompt is only supposed to be shown at page load. Is there any neat way that you could prevent loading the report component until the prompt has been adressed? If you could do that and set the correct value before rendering the component, I imagine you wouldn't need the hacky key-changing
const storedReport = sessionStorage.getItem( | ||
`meeting-${meetingId}-report`, | ||
); | ||
if (!storedReport || storedReport.length < newReportValue.length) { |
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.
Thoughts on doing this based on a timestamp rather than the length of the report? Even though this probably works in the majority of the cases, a longer report doesn't always mean a newer one
For me it does not appear to work properly.. When typing something into the report and then leaving the page without saving, I can see that it is saved to sessionStorage, however when I re-enter the meeting or meeting/edit I am not getting the prompt to get the data from sessionStorage. I also found that if something is added to sessionStorage I don't seem to be able to replace this. |
`meeting-${meeting?.id}-report`, | ||
); | ||
if (!storedReport || storedReport.length < reportValue.length) { | ||
sessionStorage.setItem(`meeting-${meeting?.id}-report`, reportValue); |
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.
Minor/unlikely security concern. A user who has signed out after editing a report (e.g. on another persons computer) will still have this data in their session storage. A quick fix here is clearing it whenever a user signs out, i.e. add a sessionStorage.clear()
here
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.
You can just clear it when this component unmounts
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.
I'm just adding it to logout button and then merging.
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.
Why add it to an action that has nothing to do with this?
78e9126
to
a6aa9ee
Compare
a6aa9ee
to
95a6007
Compare
Description
This adds localstorage saving to meeting reports.
Onchange, if the value is longer than the previous value it will override the localstorage value (or if the localstorage key does not exist).
This was re-opened as we decided at the meeting to go for this approach after consulting the router wiki.
Result
Caution
Make sure your images do not contain any real user information.
Testing
Resolves ABA-740